-
Notifications
You must be signed in to change notification settings - Fork 52.8k
fix(dashboard): fail closed on unknown gateway ownership, isolate per-profile maintenance #110405
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d3c7b66
c323b9a
4122407
86c6e0e
49658d1
c62130f
41e51a0
632e1d5
ca23056
7bd58c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -215,10 +215,125 @@ def _open_session_db_for_profile(profile: Optional[str], *, read_only: bool): | |
| _last_auto_archive_check: Dict[str, float] = {} | ||
|
|
||
|
|
||
| def _strict_gateway_health_probe(): | ||
| """``(alive, body)`` for the configured cross-container gateway, raising on "couldn't tell". | ||
|
|
||
| ``_probe_gateway_health`` collapses every failure — DNS, timeout, refused, non-200 — into | ||
| ``(False, None)``, which ``resolve_gateway_liveness`` cannot distinguish from a gateway that | ||
| is genuinely down. For a *status page* that is fine; for this gate it is not, because | ||
| "unreachable" would license a second writer on a store a live remote gateway owns. So a | ||
| configured endpoint that does not positively confirm liveness raises, which | ||
| ``resolve_gateway_liveness`` records as ``probe_error`` and this gate treats as owned. | ||
|
|
||
| The cost is deliberate: with ``GATEWAY_HEALTH_URL`` set and the remote gateway actually down, | ||
| the dashboard stops sweeping rather than risk the tear. A skipped sweep costs one archive | ||
| interval (#110405 review). | ||
| """ | ||
| from hermes_cli.web_server_gateway import _probe_gateway_health | ||
|
|
||
| alive, body = _probe_gateway_health() | ||
| if alive: | ||
| return True, body | ||
| raise RuntimeError("configured gateway health endpoint did not confirm liveness") | ||
|
|
||
|
|
||
| def _gateway_owns_home(home: Path) -> bool: | ||
| """True when a gateway owns the store at ``home`` *or* ownership can't be determined. | ||
|
|
||
| ``_check_gateway_running`` returns only ``GatewayLiveness.running`` and drops | ||
| ``probe_error``, which is the field that exists to tell "down" from "unknown" | ||
| (``gateway/status.py``). A caller that reads only ``.running`` treats an unreadable PID file | ||
| or an unflockable lock as "no gateway" and opens a second writer — the exact tear this gate | ||
| exists to prevent. So resolve the liveness here and count unknown as owned. | ||
|
|
||
| Two rungs need more than the default wiring, both found in review on #110405: | ||
|
|
||
| * ``get_running_pid()`` normalises malformed/unreadable identity metadata to ``None``, so an | ||
| ACTIVE runtime lock whose PID or lock record is corrupt reported ``running=False, | ||
| probe_error=False``. ``get_running_pid_identity_strict()`` raises on ambiguous state | ||
| instead, and (like the non-strict call with ``cleanup_stale=False``) never unlinks another | ||
| profile's PID file. | ||
| * The cross-container health rung is only consulted when a probe is passed. In a split | ||
| gateway/dashboard deployment ``GATEWAY_HEALTH_URL`` can be the ONLY evidence the gateway is | ||
| live, since local PID and runtime files are absent entirely. | ||
| """ | ||
| from gateway.status import get_running_pid_identity_strict, resolve_gateway_liveness | ||
| from hermes_cli.web_server import _GATEWAY_HEALTH_URL | ||
|
|
||
| def _pid_probe(path): | ||
| identity = get_running_pid_identity_strict(Path(path)) | ||
| return identity[0] if identity else None | ||
|
|
||
| liveness = resolve_gateway_liveness( | ||
| profile_dir=home, use_cache=False, pid_probe=_pid_probe, | ||
| health_probe=_strict_gateway_health_probe if _GATEWAY_HEALTH_URL else None) | ||
| return bool(liveness.running or liveness.probe_error) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Bug] (blocking, P1) This gate still fails open for a held runtime lock when the identity metadata is malformed or unreadable. |
||
|
|
||
|
|
||
| def _satellite_owned_by_multiplexer(name: str) -> bool: | ||
| """True when the default multiplexer owns satellite ``name``'s store, OR we can't tell. | ||
|
|
||
| ``_served_by_running_multiplexer`` / ``named_profile_served_by_running_multiplexer`` | ||
| convert every probe failure into ``False``, so an unreadable or malformed | ||
| ``gateway.pid`` / ``gateway_state.json`` under a live default gateway reads as | ||
| "nobody serves this profile" and the dashboard opens a second writer into a store the | ||
| multiplexer is holding (#110405 review). This is the tri-state version: it only returns | ||
| False when the default multiplexer is *positively* known not to own the store. | ||
|
|
||
| Raises rather than swallowing — the caller treats an exception as owned. | ||
| """ | ||
| from hermes_cli.gateway_multiplex_served import recorded_served_profiles | ||
| from hermes_cli.profiles import normalize_profile_name | ||
| from hermes_constants import get_default_hermes_root | ||
|
|
||
| default_root = Path(get_default_hermes_root()) | ||
| if not _gateway_owns_home(default_root): | ||
| return False # the default multiplexer is definitively down: nothing holds that writer | ||
| # It is up, or its liveness is unknown. An authoritative served list settles it; None means | ||
| # "no record" (stopped, pre-multiplex writer, or an unreadable/malformed record), which under a | ||
| # live-or-unknown multiplexer is exactly the ambiguity that must fail closed. | ||
| recorded = recorded_served_profiles(default_root) | ||
| if recorded is None: | ||
| return True | ||
| return normalize_profile_name(name) in {normalize_profile_name(p) for p in recorded} | ||
|
|
||
|
|
||
| def _auto_archive_owned_by_gateway(profile: Optional[str]) -> bool: | ||
| """True when a live gateway already owns ``profile``'s session store. | ||
|
|
||
| The gateway runs its own ``maybe_auto_archive`` timer, so standing down skips | ||
| nothing — but opening a *writable* ``SessionDB`` here and closing it tears down the | ||
| WAL generation the gateway is holding (SQLite checkpoints on close and unlinks | ||
| ``-wal``/``-shm`` as the apparent last connection), stranding it on deleted inodes | ||
| behind ``DeletedWalGenerationError`` (#109727, #107688, #100896). | ||
|
|
||
| Fails closed in every direction: an unknown liveness answer, an unresolvable | ||
| profile, or a raising multiplexer probe all count as owned. A skipped sweep costs | ||
| one archive interval; a wrong "not owned" costs the gateway its WAL. | ||
| """ | ||
| try: | ||
| if not profile: | ||
| from hermes_constants import get_hermes_home | ||
|
|
||
| return _gateway_owns_home(get_hermes_home()) | ||
|
|
||
| from hermes_cli.web_server_cron import _cron_profile_home | ||
|
|
||
| name, home = _cron_profile_home(profile) | ||
| if _gateway_owns_home(Path(home)): | ||
| return True | ||
| # A served satellite writes no gateway.pid of its own; the live default | ||
| # multiplexer holds its writer and (since this change) sweeps it too. | ||
| return bool(name != "default" and _satellite_owned_by_multiplexer(name)) | ||
| except Exception: | ||
| return True | ||
|
|
||
|
|
||
| def _maybe_auto_archive_for_profile(profile: Optional[str]) -> None: | ||
| """Config-gated stale-session auto-archive for ``profile``; never raises. | ||
| ``hermes serve`` runs neither CLI nor gateway startup hooks, so this | ||
| session-list trigger is what makes ``sessions.auto_archive`` work there.""" | ||
| session-list trigger is what makes ``sessions.auto_archive`` work there — | ||
| but only when no gateway owns the store (see ``_auto_archive_owned_by_gateway``).""" | ||
| try: | ||
| key = profile or "" | ||
| now = time.monotonic() | ||
|
|
@@ -242,17 +357,20 @@ def _maybe_auto_archive_for_profile(profile: Optional[str]) -> None: | |
| reset_hermes_home_override(_home_token) | ||
| if not cfg.get("auto_archive", False): | ||
| return | ||
| from hermes_cli.profiles import _check_gateway_running | ||
|
|
||
| # A live gateway owns this profile's store and runs the same sweep on its own | ||
| # housekeeping tick ("state.db maintenance tick" in gateway/run.py, profile-scoped so a | ||
| # multiplexed secondary's store is swept too). Opening it WRITABLE from `hermes | ||
| # serve` adds a second writer to a database another process is already archiving, | ||
| # for zero extra coverage (#110405). `_check_gateway_running` is the canonical | ||
| # per-profile predicate (`_maybe_run_skill_maintenance` below uses it): its | ||
| # multiplexer rung catches a served secondary, which owns no gateway.pid or lock | ||
| # of its own and a bare lock-file probe would report stopped. | ||
| if _check_gateway_running(profile_home): | ||
| # multiplexed secondary's store is swept too). Opening it WRITABLE from `hermes serve` | ||
| # adds a second writer to a database another process is already archiving, for zero | ||
| # extra coverage (#110405). | ||
| # | ||
| # Deliberately NOT `_check_gateway_running`: that exposes only `GatewayLiveness.running` | ||
| # and drops `probe_error`, and `_served_by_running_multiplexer` turns probe failures into | ||
| # False. So an unreadable gateway.pid or a malformed gateway_state.json under a LIVE | ||
| # gateway reads as "no owner" and the second writer is opened anyway — both reproduced in | ||
| # review on #110405. This gate resolves the liveness itself, keeps the unknown state, and | ||
| # treats unknown as owned. | ||
| if _auto_archive_owned_by_gateway(profile): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Bug] (blocking, P1) Use the already resolved |
||
| _log.debug("auto-archive stood down: gateway owns profile %r", profile or "default") | ||
| return | ||
| db = _open_session_db_for_profile(profile, read_only=False) | ||
| try: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| """One profile's broken store must not abandon the profiles after it in the same tick. | ||
|
|
||
| ``_for_each_served_profile`` runs the maintenance chore once per served profile with no | ||
| boundary between them, and ``_housekeeping_chore`` only catches at the tick level — so an | ||
| unreadable store would strand every profile that follows it. That state is reachable: | ||
| ``GatewayRunner._init_session_db()`` deliberately tolerates a failed primary-store init and | ||
| keeps running, and the dashboard stands down for served satellites (#109727), leaving the | ||
| multiplexer as their only sweeper. Review P2 on #110405. | ||
| """ | ||
| from pathlib import Path | ||
|
|
||
| import pytest | ||
|
|
||
|
|
||
| class _FakeDB: | ||
| def __init__(self, path, swept): | ||
| self.path, self._swept = path, swept | ||
|
|
||
| def maybe_auto_archive(self, **kwargs): | ||
| self._swept.append((self.path, kwargs["idle_days"])) | ||
|
|
||
| def maybe_auto_prune_and_vacuum(self, **kwargs): | ||
| pass | ||
|
|
||
|
|
||
| class _Runner: | ||
| class config: | ||
| multiplex_profiles = True | ||
| sessions_dir = None | ||
|
|
||
|
|
||
| def _write_profile(home: Path, days) -> None: | ||
| home.mkdir(parents=True, exist_ok=True) | ||
| (home / "config.yaml").write_text( | ||
| f"sessions:\n auto_archive: true\n auto_archive_days: {days}\n", encoding="utf-8") | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def homes(tmp_path, monkeypatch): | ||
| launch, sat = tmp_path / "launch", tmp_path / "profiles" / "work" | ||
| _write_profile(launch, 3) | ||
| _write_profile(sat, 9) | ||
| monkeypatch.setenv("HERMES_HOME", str(launch)) | ||
| return launch, sat | ||
|
|
||
|
|
||
| def _serve(monkeypatch, *profiles): | ||
| import gateway.run as run_mod | ||
|
|
||
| monkeypatch.setattr(run_mod, "_multiplex_profile_homes", lambda config: list(profiles)) | ||
|
|
||
|
|
||
| def _tick(runner=None): | ||
| from gateway.run import _housekeeping_state_db_maintenance | ||
| from gateway.run_profile_reconcile import profile_scoped_chore | ||
|
|
||
| profile_scoped_chore(runner or _Runner(), _housekeeping_state_db_maintenance)() | ||
|
|
||
|
|
||
| def test_a_broken_store_does_not_strand_the_profiles_after_it(homes, monkeypatch): | ||
| launch, sat = homes | ||
| swept = [] | ||
| _serve(monkeypatch, ("default", launch), ("work", sat)) | ||
|
|
||
| import hermes_state_registry as reg | ||
|
|
||
| from hermes_constants import get_hermes_home | ||
|
|
||
| def _acquire(*a, **k): | ||
| home = get_hermes_home() | ||
| if home == launch: | ||
| raise OSError("launch store unavailable") | ||
| return _FakeDB(home / "state.db", swept) | ||
|
|
||
| monkeypatch.setattr(reg, "acquire", _acquire) | ||
| monkeypatch.setattr(reg, "release_or_close", lambda db: None) | ||
|
|
||
| _tick() | ||
|
|
||
| assert [p for p, _ in swept] == [sat / "state.db"], \ | ||
| f"the satellite after the broken launch store must still be swept; swept={swept}" | ||
|
|
||
|
|
||
| def test_the_chore_never_raises(homes, monkeypatch): | ||
| """Asserted directly on the chore, not just through the loop.""" | ||
| import hermes_state_registry as reg | ||
|
|
||
| from gateway.run import _housekeeping_state_db_maintenance | ||
|
|
||
| def _boom(*a, **k): | ||
| raise RuntimeError("store exploded") | ||
|
|
||
| monkeypatch.setattr(reg, "acquire", _boom) | ||
|
|
||
| _housekeeping_state_db_maintenance() # must not raise | ||
|
|
||
|
|
||
| def test_healthy_profiles_are_each_swept_with_their_own_config(homes, monkeypatch): | ||
| """The isolation must not swallow the normal path: both profiles still sweep, each under | ||
| its own config (the scoping itself is main's `profile_scoped_chore`).""" | ||
| launch, sat = homes | ||
| swept = [] | ||
| _serve(monkeypatch, ("default", launch), ("work", sat)) | ||
|
|
||
| import hermes_state_registry as reg | ||
|
|
||
| from hermes_constants import get_hermes_home | ||
|
|
||
| monkeypatch.setattr(reg, "acquire", lambda *a, **k: _FakeDB(get_hermes_home() / "state.db", swept)) | ||
| monkeypatch.setattr(reg, "release_or_close", lambda db: None) | ||
|
|
||
| _tick() | ||
|
|
||
| by_path = {p: d for p, d in swept} | ||
| assert by_path.get(launch / "state.db") == 3.0 | ||
| assert by_path.get(sat / "state.db") == 9.0, "satellite must use its OWN auto_archive_days" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Bug] (blocking, P1) This ownership check skips the configured cross-container health rung. In a split gateway/dashboard deployment,
GATEWAY_HEALTH_URLcan be the only evidence that the remote gateway is live while local PID and runtime files are absent. I reproduced the divergence at this head:resolve_gateway_liveness(..., health_probe=_probe_gateway_health)returnedrunning=Truewith sourcehealth, while_gateway_owns_home()returnedFalsefor the same home._maybe_auto_archive_for_profile()can then open and close a writableSessionDBon the shared store, recreating the WAL-generation hazard this gate is intended to prevent. Please include the configured health probe in this ownership decision. A configured probe that cannot establish absence should fail closed here. Add a regression where only remote health reports ownership.