fix: harden SessionStore — per-record timestamp handling, atomic writes, robust quarantine

docs/superpowers/plans/2026-07-07-wabot-modernization.md

@@ -371,6 +371,62 @@ class TestSessionStore:
 
         assert default_store_path().name == "sessions.json"
         assert "wabot" in str(default_store_path())
+
+    def test_naive_created_at_is_treated_as_utc_not_crash(self, tmp_path):
+        # legacy/hand-written records may carry naive timestamps
+        import json
+
+        path = tmp_path / "sessions.json"
+        record = make_record()
+        data = {"scraper1": {**record.__dict__, "created_at": "2999-01-01T00:00:00"}}
+        path.write_text(json.dumps(data))
+        store = SessionStore(path=path)
+        assert store.get("scraper1") is not None  # fresh: kept, assumed UTC
+
+    def test_garbage_created_at_evicts_only_that_record(self, tmp_path):
+        import json
+
+        path = tmp_path / "sessions.json"
+        good, bad = make_record(name="good"), make_record(name="bad")
+        data = {
+            "good": good.__dict__,
+            "bad": {**bad.__dict__, "created_at": "not-a-date"},
+        }
+        path.write_text(json.dumps(data))
+        store = SessionStore(path=path)
+        assert store.names() == ["good"]  # no crash, bad evicted, file rewritten
+        assert "not-a-date" not in path.read_text()
+
+    def test_null_created_at_evicts_only_that_record(self, tmp_path):
+        import json
+
+        path = tmp_path / "sessions.json"
+        data = {"broken": {**make_record(name="broken").__dict__, "created_at": None}}
+        path.write_text(json.dumps(data))
+        assert SessionStore(path=path).names() == []
+
+    def test_unknown_record_fields_are_ignored_not_fatal(self, tmp_path):
+        # a newer wabot may add fields; older versions must not quarantine the store
+        import json
+
+        path = tmp_path / "sessions.json"
+        data = {"scraper1": {**make_record().__dict__, "future_field": 42}}
+        path.write_text(json.dumps(data))
+        assert SessionStore(path=path).get("scraper1") is not None
+
+    def test_repeat_corruption_with_existing_bad_file_does_not_crash(self, tmp_path):
+        path = tmp_path / "sessions.json"
+        (tmp_path / "sessions.json.bad").write_text("old corruption")
+        path.write_text("{ corrupt again")
+        store = SessionStore(path=path)
+        assert store.load() == {}
+        store.save(make_record())
+        assert store.get("scraper1") is not None
+
+    def test_write_is_atomic_no_tmp_leftover(self, tmp_path):
+        store = SessionStore(path=tmp_path / "sessions.json")
+        store.save(make_record())
+        assert not (tmp_path / "sessions.json.tmp").exists()
 ```
 
 - [ ] **Step 2: Run tests to verify they fail**
@@ -395,6 +451,7 @@ from __future__ import annotations
 import dataclasses
 import json
 import logging
+import os
 from dataclasses import dataclass
 from datetime import datetime, timedelta, timezone
 from pathlib import Path
@@ -417,6 +474,20 @@ class SessionRecord:
     service_port: int | None = None
 
 
+_RECORD_FIELDS = {field.name for field in dataclasses.fields(SessionRecord)}
+
+
+def _parse_created_at(value) -> datetime | None:
+    """Parse a record timestamp; None means unusable (treat as stale)."""
+    try:
+        created = datetime.fromisoformat(value)
+    except (ValueError, TypeError):
+        return None
+    if created.tzinfo is None:
+        created = created.replace(tzinfo=timezone.utc)
+    return created
+
+
 def default_store_path() -> Path:
     return Path(platformdirs.user_data_dir("wabot")) / "sessions.json"
 
@@ -433,16 +504,22 @@ class SessionStore:
         if not self.path.exists():
             return {}
         try:
-            raw = json.loads(self.path.read_text())
-            records = {name: SessionRecord(**data) for name, data in raw.items()}
-        except (json.JSONDecodeError, TypeError, AttributeError):
-            bad = self.path.with_name(self.path.name + ".bad")
-            self.path.rename(bad)
-            LOGGER.warning("corrupt session store moved to %s; starting fresh", bad)
+            raw = json.loads(self.path.read_text(encoding="utf-8"))
+            records = {
+                name: SessionRecord(**{k: v for k, v in data.items() if k in _RECORD_FIELDS})
+                for name, data in raw.items()
+            }
+        except (json.JSONDecodeError, TypeError, AttributeError, OSError):
+            self._quarantine()
             return {}
         fresh = {}
         for name, record in records.items():
-            created = datetime.fromisoformat(record.created_at)
+            created = _parse_created_at(record.created_at)
+            if created is None:
+                LOGGER.warning(
+                    "evicting session %r with unusable created_at %r", name, record.created_at
+                )
+                continue
             if datetime.now(timezone.utc) - created >= self.max_age:
                 LOGGER.info("evicting stale session %r (created %s)", name, record.created_at)
                 continue
@@ -451,6 +528,14 @@ class SessionStore:
             self._write(fresh)
         return fresh
 
+    def _quarantine(self) -> None:
+        bad = self.path.with_name(self.path.name + ".bad")
+        try:
+            self.path.replace(bad)  # replace, not rename: works when .bad exists
+            LOGGER.warning("corrupt session store moved to %s; starting fresh", bad)
+        except OSError as ex:
+            LOGGER.warning("could not quarantine corrupt store %s: %s", self.path, ex)
+
     def get(self, name: str) -> SessionRecord | None:
         return self.load().get(name)
 
@@ -468,9 +553,14 @@ class SessionStore:
         return sorted(self.load())
 
     def _write(self, records: dict[str, SessionRecord]) -> None:
+        # NOTE: save() is read-modify-write with no cross-process lock; at this
+        # library's scale (a handful of sessions on human timescales) a lost
+        # update is acceptable. A torn/partial file is NOT — hence atomic replace.
         self.path.parent.mkdir(parents=True, exist_ok=True)
         payload = {name: dataclasses.asdict(rec) for name, rec in records.items()}
-        self.path.write_text(json.dumps(payload, indent=2))
+        tmp = self.path.with_name(self.path.name + ".tmp")
+        tmp.write_text(json.dumps(payload, indent=2), encoding="utf-8")
+        os.replace(tmp, self.path)
 ```
 
 Note the corrupt-file test writes `sessions.json` + `.bad` → `sessions.json.bad`, which is why `_write` uses `with_name(self.path.name + ".bad")` (not `with_suffix`, which would replace `.json`).
@@ -478,7 +568,7 @@ Note the corrupt-file test writes `sessions.json` + `.bad` → `sessions.json.ba
 - [ ] **Step 4: Run tests to verify they pass**
 
 Run: `uv run pytest tests/unit/test_sessions.py -v`
-Expected: 8 passed
+Expected: 14 passed
 
 - [ ] **Step 5: Commit**
 
@@ -1052,7 +1142,7 @@ Run: `uv run pytest tests/unit/test_reattach.py -v`
 Expected: 5 passed
 
 Run: `uv run pytest`
-Expected: all unit tests so far pass (pacing 6, sessions 8, hosts 17, reattach 5 = 36)
+Expected: all unit tests so far pass (pacing 7, sessions 14, hosts 17, reattach 5 = 43)
 
 - [ ] **Step 5: Commit**
 
@@ -2577,7 +2667,7 @@ Run: `uv run pytest tests/unit/test_api.py -v`
 Expected: 11 passed
 
 Run: `uv run pytest`
-Expected: full unit suite passes (97 tests: pacing 6, sessions 8, hosts 17, reattach 5, fields 11, page 23, screenshot 3, browser 13, api 11)
+Expected: full unit suite passes (104 tests: pacing 7, sessions 14, hosts 17, reattach 5, fields 11, page 23, screenshot 3, browser 13, api 11)
 
 - [ ] **Step 5: Commit**
 

src/wabot/sessions.py

@@ -10,6 +10,7 @@ from __future__ import annotations
 import dataclasses
 import json
 import logging
+import os
 from dataclasses import dataclass
 from datetime import datetime, timedelta, timezone
 from pathlib import Path
@@ -32,6 +33,20 @@ class SessionRecord:
     service_port: int | None = None
 
 
+_RECORD_FIELDS = {field.name for field in dataclasses.fields(SessionRecord)}
+
+
+def _parse_created_at(value) -> datetime | None:
+    """Parse a record timestamp; None means unusable (treat as stale)."""
+    try:
+        created = datetime.fromisoformat(value)
+    except (ValueError, TypeError):
+        return None
+    if created.tzinfo is None:
+        created = created.replace(tzinfo=timezone.utc)
+    return created
+
+
 def default_store_path() -> Path:
     return Path(platformdirs.user_data_dir("wabot")) / "sessions.json"
 
@@ -48,16 +63,22 @@ class SessionStore:
         if not self.path.exists():
             return {}
         try:
-            raw = json.loads(self.path.read_text())
-            records = {name: SessionRecord(**data) for name, data in raw.items()}
-        except (json.JSONDecodeError, TypeError, AttributeError):
-            bad = self.path.with_name(self.path.name + ".bad")
-            self.path.rename(bad)
-            LOGGER.warning("corrupt session store moved to %s; starting fresh", bad)
+            raw = json.loads(self.path.read_text(encoding="utf-8"))
+            records = {
+                name: SessionRecord(**{k: v for k, v in data.items() if k in _RECORD_FIELDS})
+                for name, data in raw.items()
+            }
+        except (json.JSONDecodeError, TypeError, AttributeError, OSError):
+            self._quarantine()
             return {}
         fresh = {}
         for name, record in records.items():
-            created = datetime.fromisoformat(record.created_at)
+            created = _parse_created_at(record.created_at)
+            if created is None:
+                LOGGER.warning(
+                    "evicting session %r with unusable created_at %r", name, record.created_at
+                )
+                continue
             if datetime.now(timezone.utc) - created >= self.max_age:
                 LOGGER.info("evicting stale session %r (created %s)", name, record.created_at)
                 continue
@@ -66,6 +87,14 @@ class SessionStore:
             self._write(fresh)
         return fresh
 
+    def _quarantine(self) -> None:
+        bad = self.path.with_name(self.path.name + ".bad")
+        try:
+            self.path.replace(bad)  # replace, not rename: works when .bad exists
+            LOGGER.warning("corrupt session store moved to %s; starting fresh", bad)
+        except OSError as ex:
+            LOGGER.warning("could not quarantine corrupt store %s: %s", self.path, ex)
+
     def get(self, name: str) -> SessionRecord | None:
         return self.load().get(name)
 
@@ -83,6 +112,11 @@ class SessionStore:
         return sorted(self.load())
 
     def _write(self, records: dict[str, SessionRecord]) -> None:
+        # NOTE: save() is read-modify-write with no cross-process lock; at this
+        # library's scale (a handful of sessions on human timescales) a lost
+        # update is acceptable. A torn/partial file is NOT — hence atomic replace.
         self.path.parent.mkdir(parents=True, exist_ok=True)
         payload = {name: dataclasses.asdict(rec) for name, rec in records.items()}
-        self.path.write_text(json.dumps(payload, indent=2))
+        tmp = self.path.with_name(self.path.name + ".tmp")
+        tmp.write_text(json.dumps(payload, indent=2), encoding="utf-8")
+        os.replace(tmp, self.path)

tests/unit/test_sessions.py

@@ -67,3 +67,59 @@ class TestSessionStore:
 
         assert default_store_path().name == "sessions.json"
         assert "wabot" in str(default_store_path())
+
+    def test_naive_created_at_is_treated_as_utc_not_crash(self, tmp_path):
+        # legacy/hand-written records may carry naive timestamps
+        import json
+
+        path = tmp_path / "sessions.json"
+        record = make_record()
+        data = {"scraper1": {**record.__dict__, "created_at": "2999-01-01T00:00:00"}}
+        path.write_text(json.dumps(data))
+        store = SessionStore(path=path)
+        assert store.get("scraper1") is not None  # fresh: kept, assumed UTC
+
+    def test_garbage_created_at_evicts_only_that_record(self, tmp_path):
+        import json
+
+        path = tmp_path / "sessions.json"
+        good, bad = make_record(name="good"), make_record(name="bad")
+        data = {
+            "good": good.__dict__,
+            "bad": {**bad.__dict__, "created_at": "not-a-date"},
+        }
+        path.write_text(json.dumps(data))
+        store = SessionStore(path=path)
+        assert store.names() == ["good"]  # no crash, bad evicted, file rewritten
+        assert "not-a-date" not in path.read_text()
+
+    def test_null_created_at_evicts_only_that_record(self, tmp_path):
+        import json
+
+        path = tmp_path / "sessions.json"
+        data = {"broken": {**make_record(name="broken").__dict__, "created_at": None}}
+        path.write_text(json.dumps(data))
+        assert SessionStore(path=path).names() == []
+
+    def test_unknown_record_fields_are_ignored_not_fatal(self, tmp_path):
+        # a newer wabot may add fields; older versions must not quarantine the store
+        import json
+
+        path = tmp_path / "sessions.json"
+        data = {"scraper1": {**make_record().__dict__, "future_field": 42}}
+        path.write_text(json.dumps(data))
+        assert SessionStore(path=path).get("scraper1") is not None
+
+    def test_repeat_corruption_with_existing_bad_file_does_not_crash(self, tmp_path):
+        path = tmp_path / "sessions.json"
+        (tmp_path / "sessions.json.bad").write_text("old corruption")
+        path.write_text("{ corrupt again")
+        store = SessionStore(path=path)
+        assert store.load() == {}
+        store.save(make_record())
+        assert store.get("scraper1") is not None
+
+    def test_write_is_atomic_no_tmp_leftover(self, tmp_path):
+        store = SessionStore(path=tmp_path / "sessions.json")
+        store.save(make_record())
+        assert not (tmp_path / "sessions.json.tmp").exists()