diff --git a/docs/superpowers/plans/2026-07-07-wabot-modernization.md b/docs/superpowers/plans/2026-07-07-wabot-modernization.md index 323dd7e..c52f0df 100644 --- a/docs/superpowers/plans/2026-07-07-wabot-modernization.md +++ b/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** diff --git a/src/wabot/sessions.py b/src/wabot/sessions.py index 155f0f2..89a83bf 100644 --- a/src/wabot/sessions.py +++ b/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) diff --git a/tests/unit/test_sessions.py b/tests/unit/test_sessions.py index 194f20b..c29433d 100644 --- a/tests/unit/test_sessions.py +++ b/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()