fix(cacher): resolve cache expiry by unix time

tests/core/test_cacher.py

@@ -0,0 +1,100 @@
+import time
+from datetime import datetime, timedelta, timezone
+from pathlib import Path
+
+import jsonpickle
+import jwt
+import pytest
+
+from unshackle.core import cacher
+from unshackle.core.cacher import Cacher
+
+CLOSE = timedelta(seconds=5)
+
+
+def set_zone(monkeypatch: pytest.MonkeyPatch, zone: str) -> None:
+    monkeypatch.setenv("TZ", zone)
+    time.tzset()
+
+
+@pytest.fixture
+def zone(monkeypatch: pytest.MonkeyPatch):  # type: ignore[no-untyped-def]
+    """Let a test change the process time zone, and put it back afterwards."""
+    if not hasattr(time, "tzset"):
+        pytest.skip("needs time.tzset")
+    yield lambda name: set_zone(monkeypatch, name)
+    monkeypatch.undo()
+    time.tzset()
+
+
+@pytest.fixture
+def cache(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Cacher:
+    monkeypatch.setattr(cacher.config.directories, "cache", tmp_path)
+    return Cacher("TAG")
+
+
+def test_seconds_until_expiration_ignore_the_1970_utc_offset(zone) -> None:  # type: ignore[no-untyped-def]
+    zone("Asia/Kathmandu")
+    if time.localtime(0).tm_gmtoff == time.localtime().tm_gmtoff:
+        pytest.skip("zone data missing: the 1970 offset equals the current one")
+    assert abs(Cacher.resolve_datetime(283) - (datetime.now() + timedelta(seconds=283))) < CLOSE
+
+
+@pytest.mark.parametrize("value", [283, 283.5, "283"])
+def test_small_number_is_seconds_until_expiration(value: object) -> None:
+    assert abs(Cacher.resolve_datetime(value) - (datetime.now() + timedelta(seconds=283))) < CLOSE  # type: ignore[arg-type]
+
+
+@pytest.mark.parametrize("offset", [-3600, 3600])
+def test_unix_timestamp_is_absolute_also_when_it_is_in_the_past(offset: int) -> None:
+    stamp = int(time.time()) + offset
+    assert Cacher.resolve_datetime(stamp) == datetime.fromtimestamp(stamp)
+    assert Cacher.resolve_datetime(stamp * 1000) == datetime.fromtimestamp(stamp)
+
+
+def test_iso_string_with_utc_offset_becomes_naive_local_time() -> None:
+    moment = datetime.now(timezone.utc).replace(microsecond=0) + timedelta(hours=1)
+    expected = datetime.fromtimestamp(moment.timestamp())
+    assert Cacher.resolve_datetime(moment.isoformat()) == expected
+    assert Cacher.resolve_datetime(moment.strftime("%Y-%m-%dT%H:%M:%SZ")) == expected
+    assert Cacher.resolve_datetime(moment) == expected
+
+
+@pytest.mark.parametrize("value", [float("inf"), 10**17, "not a time"])
+def test_unusable_value_raises_value_error(value: object) -> None:
+    with pytest.raises(ValueError):
+        Cacher.resolve_datetime(value)  # type: ignore[arg-type]
+
+
+def test_zero_seconds_expires_now(cache: Cacher) -> None:
+    entry = cache.get("key")
+    entry.set({"a": 1}, expiration=0)
+    assert entry.expired
+
+
+def test_expired_jwt_is_stored_as_expired(cache: Cacher) -> None:
+    entry = cache.get("key")
+    entry.set(jwt.encode({"exp": int(time.time()) - 60}, "k" * 32, algorithm="HS256"))
+    assert entry.expired
+
+
+def test_expiration_survives_a_change_of_time_zone(cache: Cacher, zone) -> None:  # type: ignore[no-untyped-def]
+    zone("UTC")
+    cache.get("key").set({"a": 1}, expiration=300)
+    zone("America/Denver")
+    entry = cache.get("key")
+    assert entry.data == {"a": 1}
+    assert abs(entry.expiration - (datetime.now() + timedelta(seconds=300))) < CLOSE
+
+
+def test_entry_without_expires_at_reads_its_local_expiration(cache: Cacher) -> None:
+    entry = cache.get("key")
+    entry.set({"a": 1}, expiration=300)
+    payload = jsonpickle.loads(entry.path.read_text(encoding="utf8"))
+    del payload["expires_at"], payload["crc32"]
+    payload["crc32"] = cacher.zlib.crc32(jsonpickle.dumps(payload).encode("utf8"))
+    entry.path.write_text(jsonpickle.dumps(payload), encoding="utf8")
+
+    legacy = cache.get("key")
+    assert not legacy.expired
+    assert abs(legacy.expiration - (datetime.now() + timedelta(seconds=300))) < CLOSE

unshackle/core/cacher.py

@@ -1,7 +1,8 @@
 from __future__ import annotations
 
+import time
 import zlib
-from datetime import datetime, timedelta
+from datetime import datetime
 from os import stat_result
 from pathlib import Path
 from typing import Any, Optional, Union
@@ -13,6 +14,9 @@ from unshackle.core.config import config
 
 EXP_T = Union[datetime, str, int, float]
 
+# A number below this is seconds until expiration; at or above it, a unix timestamp (2001-09-09 onwards).
+ABSOLUTE_TIMESTAMP_MIN = 1_000_000_000
+
 
 class Cacher:
     """Cacher for Services to get and set arbitrary data with expiration dates."""
@@ -45,7 +49,8 @@ class Cacher:
 
     @property
     def expired(self) -> bool:
-        return self.expiration and self.expiration < datetime.now()
+        """Compare as unix time: wall-clock comparison is wrong for an hour at each DST change."""
+        return bool(self.expiration and self.expiration.timestamp() < time.time())
 
     def get(self, key: str, version: int = 1) -> Cacher:
         """
@@ -66,7 +71,9 @@ class Cacher:
                     f"The checksum of the Cache payload mismatched. Checksum: {checksum} !== Calculated: {calculated}"
                 )
             cache.data = data["data"]
-            cache.expiration = data["expiration"]
+            # The pickled `expiration` is local wall-clock and only right in the zone that wrote it.
+            expires_at = data.get("expires_at")
+            cache.expiration = datetime.fromtimestamp(expires_at) if expires_at is not None else data["expiration"]
             cache.version = data["version"]
             if cache.version != version:
                 raise ValueError(
@@ -84,15 +91,20 @@ class Cacher:
         """
         self.data = data
 
-        if not expiration:
+        if expiration is None:
             try:
                 expiration = jwt.decode(self.data, options={"verify_signature": False})["exp"]
             except jwt.DecodeError:
                 pass
 
-        self.expiration = self.resolve_datetime(expiration) if expiration else None
+        self.expiration = self.resolve_datetime(expiration) if expiration is not None else None
 
-        payload = {"data": self.data, "expiration": self.expiration, "version": self.version}
+        payload = {
+            "data": self.data,
+            "expiration": self.expiration,
+            "expires_at": self.expiration.timestamp() if self.expiration else None,
+            "version": self.version,
+        }
         payload["crc32"] = zlib.crc32(jsonpickle.dumps(payload).encode("utf8"))
 
         self.path.parent.mkdir(parents=True, exist_ok=True)
@@ -110,46 +122,26 @@ class Cacher:
     @staticmethod
     def resolve_datetime(timestamp: EXP_T) -> datetime:
         """
-        Change a Datetime or a Timestamp in one of several formats to an absolute Datetime.
+        Change an expiration in one of several formats to a naive local datetime.
 
-        Examples:
-            >>> now = datetime.now()
-            datetime.datetime(2022, 6, 27, 9, 49, 13, 657208)
-            >>> iso8601 = now.isoformat()
-            '2022-06-27T09:49:13.657208'
-            >>> Cacher.resolve_datetime(iso8601)
-            datetime.datetime(2022, 6, 27, 9, 49, 13, 657208)
-            >>> Cacher.resolve_datetime(iso8601 + "Z")
-            datetime.datetime(2022, 6, 27, 9, 49, 13, 657208)
-            >>> Cacher.resolve_datetime(3600)
-            datetime.datetime(2022, 6, 27, 10, 52, 50, 657208)
-            >>> Cacher.resolve_datetime('3600')
-            datetime.datetime(2022, 6, 27, 10, 52, 51, 657208)
-            >>> Cacher.resolve_datetime(7800.113)
-            datetime.datetime(2022, 6, 27, 11, 59, 13, 770208)
-
-        In the int/float examples you may notice that it did not return now + 3600 seconds
-        but rather something a bit more than that. This is because the 3600 seconds start at
-        the moment the function operates, not at the `now` variable.
+        A datetime or an ISO 8601 string is taken as given, and one with a UTC offset
+        (or a final `Z`) is converted to local time. A number is a unix timestamp, in
+        seconds or 13-digit milliseconds, or the seconds until expiration when it is
+        smaller than `ABSOLUTE_TIMESTAMP_MIN`. A numeric string is read as a number.
         """
-        if isinstance(timestamp, datetime):
-            return timestamp
         if isinstance(timestamp, str):
-            if timestamp.endswith("Z"):
-                # fromisoformat doesn't accept the final Z
-                timestamp = timestamp.split("Z")[0]
             try:
-                return datetime.fromisoformat(timestamp)
+                timestamp = datetime.fromisoformat(timestamp)
             except ValueError:
-                timestamp = float(timestamp)
+                timestamp = float(str(timestamp))
+        if isinstance(timestamp, datetime):
+            # naive local time is the one form every consumer of `expiration` can do arithmetic with
+            return timestamp.astimezone().replace(tzinfo=None) if timestamp.tzinfo else timestamp
         try:
-            if len(str(int(timestamp))) == 13:  # JS-style timestamp
+            if len(str(abs(int(timestamp)))) == 13:  # JS-style timestamp
                 timestamp /= 1000
-            timestamp = datetime.fromtimestamp(timestamp)
-        except ValueError:
+            if timestamp < ABSOLUTE_TIMESTAMP_MIN:
+                timestamp += time.time()
+            return datetime.fromtimestamp(timestamp)
+        except (ValueError, OverflowError, OSError):
             raise ValueError(f"Unrecognized Timestamp value {timestamp!r}")
-        if timestamp < datetime.now():
-            # timestamp is likely an amount of seconds til expiration
-            # or, it's an already expired timestamp which is unlikely
-            timestamp = timestamp + timedelta(seconds=datetime.now().timestamp())
-        return timestamp