diff --git a/hermes_time.py b/hermes_time.py index 4bbe7d2e2e..611f22e9ad 100644 --- a/hermes_time.py +++ b/hermes_time.py @@ -15,9 +15,10 @@ crashes due to a bad timezone string. import logging import os +import threading from datetime import datetime from hermes_constants import get_config_path -from typing import Optional +from typing import Dict, Optional, Tuple logger = logging.getLogger(__name__) @@ -28,16 +29,21 @@ except ImportError: from backports.zoneinfo import ZoneInfo # type: ignore[no-redef] # Cached state, keyed to the active timezone source. This process can multiplex -# profiles by switching HERMES_HOME, so a single unkeyed value would leak the -# first profile's timezone into later profile-scoped work. -# Call reset_cache() to force re-resolution (e.g. after config changes). -_cached_tz: Optional[ZoneInfo] = None -_cached_tz_name: Optional[str] = None -_cache_resolved: bool = False -_cache_identity: Optional[tuple[str, str]] = None +# profiles by switching HERMES_HOME (context override or env), so a single +# unkeyed process-global value would leak the first profile's timezone into +# later profile-scoped work (e.g. the desktop multiplex cron ticker persisting +# another profile's ``next_run_at`` under the backend's own timezone). +# +# Entries are published atomically under ``_cache_lock`` as one +# ``identity -> (name, ZoneInfo | None)`` mapping, so two profile-scoped +# threads racing through resolution can never publish a mixed +# identity/value pair. Each profile's resolved zone stays hot across +# multiplex switches. Call reset_cache() after in-place config changes. +_cache_lock = threading.Lock() +_tz_cache: Dict[Tuple[str, str], Tuple[str, Optional[ZoneInfo]]] = {} -def _timezone_cache_identity() -> tuple[str, str]: +def _timezone_cache_identity() -> Tuple[str, str]: """Return the active source identity for the timezone cache.""" tz_env = os.getenv("HERMES_TIMEZONE", "").strip() if tz_env: @@ -107,17 +113,25 @@ def _get_zoneinfo(name: str) -> Optional[ZoneInfo]: def get_timezone() -> Optional[ZoneInfo]: """Return the active profile's configured ZoneInfo, or None (server-local). - The cache is isolated by the active environment override or profile config - path. Call ``reset_cache()`` after editing the active config in place. + The cache is isolated by the active timezone source — the explicit + ``HERMES_TIMEZONE`` override or the active profile's config path — so a + process that multiplexes profiles (desktop cron ticker, multiplex + gateway) never reuses another profile's timezone. Call ``reset_cache()`` + after editing the active config in place. """ - global _cached_tz, _cached_tz_name, _cache_resolved, _cache_identity cache_identity = _timezone_cache_identity() - if not _cache_resolved or _cache_identity != cache_identity: - _cached_tz_name = _resolve_timezone_name() - _cached_tz = _get_zoneinfo(_cached_tz_name) - _cache_identity = cache_identity - _cache_resolved = True - return _cached_tz + with _cache_lock: + entry = _tz_cache.get(cache_identity) + if entry is not None: + return entry[1] + # Resolve outside the lock (config file I/O); publish atomically below. + name = _resolve_timezone_name() + tz = _get_zoneinfo(name) + with _cache_lock: + # First writer wins so concurrent resolvers of the SAME identity + # converge on one ZoneInfo object; a different identity's write can + # never be mixed into this one — the (name, tz) pair is one value. + return _tz_cache.setdefault(cache_identity, (name, tz))[1] def reset_cache() -> None: @@ -127,11 +141,8 @@ def reset_cache() -> None: config edit or ``HERMES_TIMEZONE`` update) to force ``get_timezone()`` / ``now()`` to read the new value instead of the value cached at first use. """ - global _cached_tz, _cached_tz_name, _cache_resolved, _cache_identity - _cached_tz = None - _cached_tz_name = None - _cache_resolved = False - _cache_identity = None + with _cache_lock: + _tz_cache.clear() def now() -> datetime: @@ -146,5 +157,3 @@ def now() -> datetime: return datetime.now(tz) # No timezone configured — use server-local (still tz-aware) return datetime.now().astimezone() - - diff --git a/tests/cron/test_cron_next_run_profile_timezone.py b/tests/cron/test_cron_next_run_profile_timezone.py new file mode 100644 index 0000000000..ff33ca59b8 --- /dev/null +++ b/tests/cron/test_cron_next_run_profile_timezone.py @@ -0,0 +1,80 @@ +"""Regression test for #97905 / carrier PR #92489. + +A multiplex ticker (desktop dashboard backend, multiplex gateway) resolves +``hermes_time`` under the process's own startup profile, then ticks OTHER +profiles' cron stores via ``set_hermes_home_override()`` + ``use_cron_store()``. +Before the profile-keyed timezone cache, the first profile's resolved zone +was process-global, so ``compute_next_run`` / ``create_job`` / ``mark_job_run`` +persisted ``next_run_at`` into the ticked profile's jobs.json with the FOREIGN +process's UTC offset — e.g. ``0 14 * * *`` for an America/New_York profile +stored as ``14:00+00:00``, which becomes due at 10:00 ET and fires four hours +early (#97905; single-profile sibling report #88220). + +The invariant pinned here: ``next_run_at`` written during a foreign-process +tick carries the JOB-OWNING profile's configured UTC offset. +""" +from datetime import datetime +from pathlib import Path +from zoneinfo import ZoneInfo + +import pytest + +import hermes_time + + +@pytest.fixture(autouse=True) +def _fresh_tz_cache(monkeypatch): + monkeypatch.delenv("HERMES_TIMEZONE", raising=False) + hermes_time.reset_cache() + yield + hermes_time.reset_cache() + + +def test_foreign_process_tick_persists_owning_profile_offset( + tmp_path, monkeypatch +): + """next_run_at persisted during a multiplex tick uses the ticked + profile's timezone, not the backend process's.""" + backend_home = tmp_path / "backend" # desktop backend's own profile: UTC + profile_a = tmp_path / "north-caribbean" # job-owning profile: Eastern + backend_home.mkdir() + profile_a.mkdir() + (backend_home / "config.yaml").write_text( + "timezone: UTC\n", encoding="utf-8" + ) + (profile_a / "config.yaml").write_text( + "timezone: America/New_York\n", encoding="utf-8" + ) + + monkeypatch.setenv("HERMES_HOME", str(backend_home)) + + # Backend process resolves its own timezone first (process startup). + assert hermes_time.now().utcoffset() == datetime.now( + ZoneInfo("UTC") + ).utcoffset() + + from hermes_constants import ( + reset_hermes_home_override, + set_hermes_home_override, + ) + from cron.jobs import create_job, load_jobs, use_cron_store + + # Exactly the scoping the multiplex ticker applies per profile + # (cron/scheduler_provider.py::_tick_profiles). + token = set_hermes_home_override(str(profile_a)) + try: + with use_cron_store(profile_a): + create_job(name="daily-2pm", prompt="x", schedule="0 14 * * *") + job = load_jobs()[0] + finally: + reset_hermes_home_override(token) + + next_run = datetime.fromisoformat(job["next_run_at"]) + expected_offset = datetime.now(ZoneInfo("America/New_York")).utcoffset() + assert next_run.utcoffset() == expected_offset, ( + f"next_run_at {job['next_run_at']} persisted with foreign offset " + f"{next_run.utcoffset()} instead of the owning profile's " + f"{expected_offset} — fires (offset delta) early (#97905)" + ) + # And the wall clock honors the cron expression in the profile's zone. + assert (next_run.hour, next_run.minute) == (14, 0) diff --git a/tests/test_timezone.py b/tests/test_timezone.py index 30ae6c628c..7a729ad24e 100644 --- a/tests/test_timezone.py +++ b/tests/test_timezone.py @@ -21,11 +21,8 @@ import hermes_time def _reset_hermes_time_cache(): - """Reset the hermes_time module cache (replacement for removed reset_cache).""" - hermes_time._cached_tz = None - hermes_time._cached_tz_name = None - hermes_time._cache_resolved = False - hermes_time._cache_identity = None + """Reset the hermes_time module cache.""" + hermes_time.reset_cache() # ========================================================================= @@ -106,6 +103,64 @@ class TestGetTimezone: monkeypatch.setenv("HERMES_HOME", str(second_home)) assert str(hermes_time.get_timezone()) == "America/New_York" + # Switching BACK must return the first profile's zone (per-identity + # entries stay hot; no single-slot ping-pong). + monkeypatch.setenv("HERMES_HOME", str(first_home)) + assert str(hermes_time.get_timezone()) == "Asia/Tokyo" + + def test_concurrent_profile_resolution_never_mixes_zones( + self, tmp_path, monkeypatch + ): + """Racing profile-scoped threads must never observe a foreign zone. + + The multiplex cron ticker lets profile-A work (mark_job_run / + compute_next_run) overlap the ticker advancing to profile B. The + cache publication must be atomic per identity: identity A can never + be paired with profile B's ZoneInfo (#97905 review finding on + PR #92489). + """ + import threading + + from hermes_constants import ( + reset_hermes_home_override, + set_hermes_home_override, + ) + + zones = {"a": "Asia/Tokyo", "b": "America/New_York"} + homes = {} + for key, zone in zones.items(): + home = tmp_path / key + home.mkdir() + (home / "config.yaml").write_text( + f"timezone: {zone}\n", encoding="utf-8" + ) + homes[key] = home + monkeypatch.delenv("HERMES_TIMEZONE", raising=False) + + errors = [] + barrier = threading.Barrier(2) + + def worker(key: str) -> None: + barrier.wait() + for _ in range(200): + token = set_hermes_home_override(str(homes[key])) + try: + tz = hermes_time.get_timezone() + if str(tz) != zones[key]: + errors.append((key, str(tz))) + return + finally: + reset_hermes_home_override(token) + + threads = [ + threading.Thread(target=worker, args=(key,)) for key in zones + ] + for t in threads: + t.start() + for t in threads: + t.join() + assert not errors, f"foreign timezone observed: {errors}" + # =========================================================================