From 8b71dc31e41cb4af20dcd47851a012bc740d03d7 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 02:49:35 -0700 Subject: [PATCH] fix(state): VACUUM admission counts live holders only; argv proves the home, not the install Independent review found the previous commit could make the unbounded-growth symptom it fixes PERMANENT, and that its argv narrowing re-opened #92401 inside a single install. - other_generations_for_path() counts only LIVE generations. A retired one is already write-fenced (StateDbReplacedError, close-time checkpoint disabled) and leaves the registry only when its holder releases -- which a gateway handle does not do before shutdown. One inode replacement (repair swap, backup restore, snapshot) therefore skipped auto-VACUUM for that path for the whole process lifetime. - _argv_scoped_to_other_home is ranked evidence now. argv[0] is the SHARED install binary for every profile on a host, so it is neutral, never proof of a hold; the process's own --hermes-home / HERMES_HOME= / --profile / -p selection decides which home it serves, and a token naming ANOTHER profile's store is tested before any own-prefix check. argv[0] also stops dismissing a holder of a store whose home is not part of an install layout (a custom HERMES_HOME is served BY the binary under ~/.hermes). - install_root comes from hermes_constants.named_profile_home, not basename(parent) == "profiles": an arbitrary /profiles// tree no longer promotes all of to "ours". - The launch profile keeps its gateway.sessions_dir override when its own store is pruned; every other served profile prunes under its own /sessions. Pruning under the wrong dir orphaned transcripts forever. - glob.escape on the request_dump__* sweep (pre-existing). Tests: the retired-generation test asserted the starvation mechanism; it is replaced by the invariant (a live sibling defers VACUUM) plus a red-on-base test that a retired, write-fenced generation does not. New argv cases cover the shared binary with -p other, another profile's store token, and a non-Hermes /profiles/ tree; the housekeeping fixture now asserts the unpinned store still resolves inside the sandbox before yielding. --- gateway/run.py | 43 +++- hermes_state_holders.py | 202 +++++++++++++----- hermes_state_registry.py | 25 ++- hermes_state_sessions.py | 5 +- .../test_housekeeping_profile_scope.py | 43 ++++ ...test_auto_vacuum_in_process_holder_gate.py | 97 ++++++--- .../test_fts_holder_instance_scope.py | 85 +++++++- .../test_remove_session_files_glob_escape.py | 23 ++ 8 files changed, 428 insertions(+), 95 deletions(-) create mode 100644 tests/hermes_state/test_remove_session_files_glob_escape.py diff --git a/gateway/run.py b/gateway/run.py index 0321b68e12..38b5c95ca4 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -3710,10 +3710,11 @@ class GatewayRunner( # state.db was never pruned or vacuumed by anybody, and the launch profile's # retention_days/auto_prune decided whether it happened at all. from gateway.run_profile_reconcile import _for_each_served_profile + _launch_sessions = _launch_sessions_dir(self.config) # resolved OUTSIDE any profile scope _housekeeping_chore( "state.db startup maintenance", lambda: _for_each_served_profile( - self, lambda _label: _housekeeping_state_db_maintenance())) + self, lambda _label: _housekeeping_state_db_maintenance(_launch_sessions))) # Checkpoint store pruning is a housekeeping chore (``_housekeeping_checkpoint_prune``), not a # constructor step: its ``git gc`` repacks the whole store (tens of seconds on a GB store) and # here it ran before the control socket, adapters and the code_sha stamp — so the first @@ -4611,7 +4612,33 @@ def _housekeeping_org_skill_sync() -> None: maybe_pull_org_skills() -def _housekeeping_state_db_maintenance() -> None: +def _launch_sessions_dir(config) -> Optional[Tuple[Path, Path]]: + """``(launch home, its configured transcript dir)``, or ``None`` when the gateway carries none. + + MUST be called outside any profile scope — ``get_hermes_home()`` is what identifies the launch + home. Consumed by :func:`_profile_sessions_dir`. + """ + sessions_dir = getattr(config, "sessions_dir", None) + if sessions_dir is None: + return None + return get_hermes_home(), Path(sessions_dir) + + +def _profile_sessions_dir(launch: Optional[Tuple[Path, Path]]) -> Path: + """Transcript dir of the profile currently in scope. + + ``gateway.sessions_dir`` overrides the LAUNCH profile's transcript dir only; every other served + profile keeps ``/sessions``. Hardcoding ``/sessions`` for the launch home too wrote + transcripts to the configured dir while the prune unlinked under the default one, orphaning + every pruned session's ``.json``/``.jsonl``/``request_dump_*`` forever. + """ + home = get_hermes_home() + if launch is not None and Path(launch[0]) == home: + return Path(launch[1]) + return home / "sessions" + + +def _housekeeping_state_db_maintenance(launch: Optional[Tuple[Path, Path]] = None) -> None: """Stale-session auto-archive plus auto-prune/VACUUM for ONE profile's state.db; both are gated by sessions.min_interval_hours (VACUUM additionally by its own throttles). Opens its own SessionDB — SQLite connections are thread-bound. @@ -4620,7 +4647,8 @@ def _housekeeping_state_db_maintenance() -> None: resolve through the active scope, so an unscoped run swept only the LAUNCH profile's store with the LAUNCH profile's retention settings and a multiplexed secondary was never archived, pruned or vacuumed by anyone — the dashboard/serve trigger defers to the gateway for every profile a - gateway owns (``web_server_sessions``).""" + gateway owns (``web_server_sessions``). *launch* carries the launch home's configured transcript + dir (:func:`_launch_sessions_dir`) so its override still governs its own profile.""" from hermes_cli.config import load_config as _load_full_config from hermes_state_registry import acquire, release_or_close _sess_cfg = (_load_full_config().get("sessions") or {}) @@ -4638,8 +4666,7 @@ def _housekeeping_state_db_maintenance() -> None: min_interval_hours=int(_sess_cfg.get("min_interval_hours", 24)), min_vacuum_interval_days=int(_sess_cfg.get("min_vacuum_interval_days", 30)), vacuum=bool(_sess_cfg.get("vacuum_after_prune", True)), - # This profile's own transcript dir, not the launch profile's ``config.sessions_dir``. - sessions_dir=get_hermes_home() / "sessions") + sessions_dir=_profile_sessions_dir(launch)) finally: release_or_close(_adb) @@ -4734,7 +4761,11 @@ def _start_gateway_housekeeping( (60, "Curator tick", profile_scoped_chore(runner, _housekeeping_curator)), (60, "Sync pull tick", profile_scoped_chore(runner, _housekeeping_skill_sync)), (60, "Org sync pull tick", profile_scoped_chore(runner, _housekeeping_org_skill_sync)), - (60, "state.db maintenance tick", profile_scoped_chore(runner, _housekeeping_state_db_maintenance)), + (60, "state.db maintenance tick", profile_scoped_chore( + runner, + # Default-bound now, i.e. OUTSIDE any profile scope: this is the launch home's override. + lambda _launch=_launch_sessions_dir(getattr(runner, "config", None)): + _housekeeping_state_db_maintenance(_launch))), (1, "Deferred FTS retry tick", _housekeeping_deferred_fts_retry), (1, "gateway housekeeping memory trim", _housekeeping_memory_trim), (1, "MCP config reconcile", _mcp_config_reconciler(runner)), diff --git a/hermes_state_holders.py b/hermes_state_holders.py index abac7d7605..aa8a40bccf 100644 --- a/hermes_state_holders.py +++ b/hermes_state_holders.py @@ -143,6 +143,116 @@ def canonical_sqlite_path(path: str) -> str: return os.path.normcase(os.path.abspath(path.removesuffix(" (deleted)"))) +_HOME_FLAGS = ("--hermes-home",) +_PROFILE_FLAGS = ("--profile", "-p") +_STATE_DB_NAMES = ("state.db", "state.db-wal", "state.db-shm") + + +def _norm_path(value: str) -> str: + return os.path.normcase(os.path.normpath(value)) + + +def _argv_flag_value(argv: Sequence[str], flags: Sequence[str]) -> Optional[str]: + """Last ``--flag X`` / ``--flag=X`` value, token-exact (``--profile timothy`` is not ``tim``).""" + value: Optional[str] = None + index, count = 0, len(argv) + while index < count: + token = argv[index] + if isinstance(token, str): + if token in flags and index + 1 < count and isinstance(argv[index + 1], str): + value = argv[index + 1] + index += 2 + continue + for flag in flags: + if token.startswith(flag + "="): + value = token[len(flag) + 1:] + break + index += 1 + return value + + +def _argv_env_home(argv: Sequence[str]) -> Optional[str]: + """``HERMES_HOME=`` env-style assignment on the argv (``env HERMES_HOME=… hermes …``).""" + for token in reversed(list(argv)): + if isinstance(token, str) and token.startswith("HERMES_HOME="): + return token[len("HERMES_HOME="):] + return None + + +def _store_install_layout(this_home: str) -> Tuple[Optional[str], Optional[str]]: + """``(, )`` for the home holding the store, else ``(None, None)``. + + Derived with the canonical ``named_profile_home`` predicate, never a ``basename == "profiles"`` + string test: an arbitrary ``/profiles//`` tree is not a Hermes install, and promoting + ```` to "ours" swallows an unrelated instance living under it — the literal two-instance + shape of #92401. The root store (``~/.hermes/state.db``) is its own root with no profile name, + so ANY named-profile selection contradicts it. + """ + try: + from hermes_constants import named_profile_home + + profile_home = named_profile_home(this_home) + if profile_home is not None: + return os.path.abspath(str(profile_home.parent.parent)), profile_home.name + if os.path.basename(this_home) == ".hermes": + return os.path.abspath(this_home), None + except Exception: # constants import/resolution must never break a holder scan + logger.debug("Could not classify the install layout of %s", this_home, exc_info=True) + return None, None + + +def _names_other_profile(normalized: str, install_root: Optional[str], our_profile: Optional[str]) -> bool: + """True when the token is under ``/profiles/`` for a name that is not ours.""" + if install_root is None: + return False + prefix = _norm_path(os.path.join(install_root, "profiles")) + os.sep + if not normalized.startswith(prefix): + return False + name = normalized[len(prefix):].split(os.sep, 1)[0] + return bool(name) and name != (os.path.normcase(our_profile) if our_profile else None) + + +def _argv_home_selection( + argv: Sequence[str], this_home: str, install_root: Optional[str], our_profile: Optional[str] +) -> Optional[str]: + """``"ours"``/``"other"``/``None`` from the process's OWN profile/home selection. + + Under one process per host the shared binary path proves nothing about which home a process + serves; its ``--hermes-home``/``HERMES_HOME=``/``--profile``/``-p`` selection does. Same + token-exact parsers ``gateway/run.py::_argv_contradicts_home`` uses. + """ + home_value = _argv_flag_value(argv, _HOME_FLAGS) or _argv_env_home(argv) + if home_value: + return "ours" if _norm_path(home_value) == _norm_path(this_home) else "other" + profile_value = _argv_flag_value(argv, _PROFILE_FLAGS) + if profile_value: + if our_profile is not None: + return "ours" if profile_value == our_profile else "other" + # Root/custom home: any explicit named profile selects a different home. + return "ours" if (install_root is not None and profile_value == "default") else "other" + return None + + +def _argv_path_tokens(argv: Sequence[str]) -> List[Tuple[int, str]]: + """``(argv index, normalized absolute path)`` for every path-bearing token.""" + tokens: List[Tuple[int, str]] = [] + for index, token in enumerate(argv): + if not isinstance(token, str): + continue + if token.startswith("/"): + path_token = token + elif token.startswith("-") and "=" in token: + # ``--db=/abs/path``-style options carry a path value; anchor on + # the text after '=' so normpath does not prepend the option. + value = token.split("=", 1)[1] + path_token = value if value.startswith("/") else None + else: + path_token = None + if path_token is not None: + tokens.append((index, _norm_path(path_token))) + return tokens + + def _argv_scoped_to_other_home(argv: Sequence[str], db_path: Path) -> bool: """Return whether argv proves the process belongs to a DIFFERENT instance. @@ -157,61 +267,55 @@ def _argv_scoped_to_other_home(argv: Sequence[str], db_path: Path) -> bool: handles). Ambiguous argv without absolute-path tokens returns False and keeps the fail-closed suspicion. - One process per host serves EVERY profile, so a token naming the install - root is not evidence of another instance when ``db_path`` is a profile - store under it: a default-launched multiplexer holds - ``/profiles//state.db`` while its argv mentions only - ````. Only a token under a DIFFERENT install root still counts. + Evidence is ranked, because one host now runs ONE process for every profile: + + 1. A token naming our state.db or a sidecar exactly — definitive, ours. + 2. The process's own ``--hermes-home``/``HERMES_HOME=``/``--profile``/``-p`` + selection — that is what decides which home a multiplexer serves. + 3. Path tokens. A token under ``/profiles/`` is another + profile's store even though it sits under our root; a token that names only + the SHARED install root is NEUTRAL (it is the same binary for every profile, + so it can neither prove nor disprove a hold); ``argv[0]`` locates the INSTALL, + not the home, so it is not other-home evidence for a store whose home is not + part of an install layout (a custom ``HERMES_HOME`` is served BY the binary + under ``~/.hermes`` — dismissing on it admits maintenance under a live writer). """ db_path_str = os.path.abspath(os.fspath(db_path)) this_home = os.path.dirname(db_path_str) - # ``/profiles//state.db`` -> ````; the multiplexer scoped to the - # install root is a candidate holder of every profile store beneath it. - profiles_dir = os.path.dirname(this_home) - install_root = ( - os.path.dirname(profiles_dir) - if os.path.basename(profiles_dir) == "profiles" - else None - ) - ours = { + install_root, our_profile = _store_install_layout(this_home) + sidecars = { os.path.normcase(candidate) - for candidate in ( - db_path_str, - db_path_str + "-wal", - db_path_str + "-shm", - this_home, - *((install_root,) if install_root else ()), - ) + for candidate in (db_path_str, db_path_str + "-wal", db_path_str + "-shm") } - own_prefixes = tuple( - os.path.normcase(root) + os.sep - for root in (this_home, *((install_root,) if install_root else ())) - ) - other_home_seen = False - for token in argv: - if not isinstance(token, str): + this_home_norm = os.path.normcase(this_home) + root_norm = os.path.normcase(install_root) if install_root else None + path_tokens = _argv_path_tokens(argv) + + if any(normalized in sidecars for _, normalized in path_tokens): + return False + selection = _argv_home_selection(argv, this_home, install_root, our_profile) + if selection == "ours": + return False + # A store whose home is not itself part of an install layout cannot be identified from the + # install location, so argv[0] alone never dismisses a holder of it. + argv0_locates_home = install_root is not None + other_home_seen = selection == "other" + for index, normalized in path_tokens: + if _names_other_profile(normalized, install_root, our_profile): + other_home_seen = True continue - if token.startswith("/"): - path_token = token - elif token.startswith("-") and "=" in token: - # ``--db=/abs/path``-style options carry a path value; anchor on - # the text after '=' so normpath does not prepend the option. - value = token.split("=", 1)[1] - path_token = value if value.startswith("/") else None - else: - path_token = None - if path_token is not None: - normalized = os.path.normcase(os.path.normpath(path_token)) - if normalized in ours or normalized.startswith(own_prefixes): - return False - if "/.hermes" in normalized or normalized.endswith("/.hermes"): - other_home_seen = True - elif os.path.basename(normalized) in ( - "state.db", - "state.db-wal", - "state.db-shm", - ): - other_home_seen = True + if normalized == this_home_norm or normalized.startswith(this_home_norm + os.sep): + return False + if root_norm is not None and ( + normalized == root_norm or normalized.startswith(root_norm + os.sep) + ): + continue # shared install root: neutral, every served profile lives under it + if index == 0 and not argv0_locates_home: + continue + if "/.hermes" in normalized or normalized.endswith("/.hermes"): + other_home_seen = True + elif os.path.basename(normalized) in _STATE_DB_NAMES: + other_home_seen = True return other_home_seen diff --git a/hermes_state_registry.py b/hermes_state_registry.py index fb6ffc4834..48d439fe57 100644 --- a/hermes_state_registry.py +++ b/hermes_state_registry.py @@ -401,25 +401,32 @@ def close_all_under(directory: str | Path) -> int: def other_generations_for_path( db_path: Path, *, exclude: Optional["SessionDB"] = None ) -> List[str]: - """Describe every OTHER SessionDB generation THIS process holds for *db_path*. + """Describe every other LIVE SessionDB generation THIS process holds for *db_path*. The registry is path-keyed, so it can answer the in-process half of "is this store quiet?" that a ``/proc`` descriptor scan structurally cannot: that scan skips our own pid, so it only - ever proves other PROCESSES are away. Retired generations count — they stay open for their - holders, and a VACUUM's TRUNCATE checkpoint retires the WAL generation underneath them. + ever proves other PROCESSES are away. + + RETIRED generations are deliberately not holders here. A generation is retired only after its + file was replaced, which is exactly when ``SessionDB`` fences it: every write raises + ``StateDbReplacedError`` and the close-time checkpoint is disabled, so it is not the live writer + this gate protects. It also leaves ``_retired`` only when its last holder releases, and a + gateway handle does not release before shutdown — counting it made ONE inode replacement + (recovery swap, backup restore, snapshot) skip auto-VACUUM for that path for the rest of the + process lifetime, turning the unbounded growth this maintenance exists to bound into a + permanent condition. """ try: path = Path(db_path).resolve() except OSError: path = Path(db_path) with _lock: - candidates = [ - ("live", generation) for generation in _generations.values() - ] + [("retired", generation) for generation in _retired.values()] return [ - f"in-process {kind} SessionDB generation (refcount {generation.refcount})" - for kind, generation in candidates - if generation.db is not exclude and generation.path == path + f"in-process live SessionDB generation (refcount {generation.refcount})" + for generation in _generations.values() + if generation.db is not exclude + and generation.path == path + and not generation.retired ] diff --git a/hermes_state_sessions.py b/hermes_state_sessions.py index 061502af4c..931303ac5d 100644 --- a/hermes_state_sessions.py +++ b/hermes_state_sessions.py @@ -2,6 +2,7 @@ flags (end/reopen/archive/pin/hide/read), model_config patching, listing and counting, delete cascades, and the auto-archive sweep.""" +import glob import json import logging import re @@ -1502,7 +1503,9 @@ class SessionSessionsMixin: return targets = [sessions_dir / f"{session_id}{suffix}" for suffix in (".json", ".jsonl")] try: - targets.extend(sessions_dir.glob(f"request_dump_{session_id}_*.json")) + # glob.escape: a session id carrying ``[`` / ``?`` / ``*`` is a PATTERN otherwise, so the + # dump sweep either matches nothing or matches another session's files. + targets.extend(sessions_dir.glob(f"request_dump_{glob.escape(session_id)}_*.json")) except OSError: pass for p in targets: diff --git a/tests/gateway/test_housekeeping_profile_scope.py b/tests/gateway/test_housekeeping_profile_scope.py index 328a024572..603218f796 100644 --- a/tests/gateway/test_housekeeping_profile_scope.py +++ b/tests/gateway/test_housekeeping_profile_scope.py @@ -59,6 +59,10 @@ def two_homes(tmp_path, monkeypatch): # Restore the import-time sentinel so an argless ``acquire()`` resolves through the scope. import hermes_state monkeypatch.setattr(hermes_state, "DEFAULT_DB_PATH", hermes_state._IMPORT_DEFAULT_DB_PATH) + # Disabling the hermetic pin is only safe while the sentinel still resolves INSIDE the sandbox: + # a resolution that escaped to the real home would have these tests writing the live store. + resolved = Path(hermes_state._default_db_path()) + assert resolved.is_relative_to(tmp_path), f"unpinned store escaped the sandbox: {resolved}" return a, b @@ -174,6 +178,45 @@ def test_multiplexed_maintenance_tick_prunes_every_served_profile_store(two_home db.close() +def test_prune_unlinks_transcripts_under_the_configured_sessions_dir(two_homes, tmp_path): + """``gateway.sessions_dir`` governs the LAUNCH profile's transcripts; others use their own home. + + Hardcoding ``/sessions`` made the prune unlink under a directory nothing writes to, so an + override left every pruned session's ``.json``/``.jsonl``/``request_dump_*`` orphaned forever. + """ + from agent.secret_scope import set_multiplex_active + from hermes_state import SessionDB + + a, b = two_homes + override = tmp_path / "custom-transcripts" + override.mkdir() + for home, transcripts in ((a, override), (b, b / "sessions")): + (home / "config.yaml").write_text( + "model:\n provider: nous\n" + "sessions:\n" + " auto_prune: true\n" + " retention_days: 0\n" + " min_interval_hours: 0\n" + " vacuum_after_prune: false\n", + encoding="utf-8") + db = SessionDB(db_path=home / "state.db") + db.create_session("old", "cli") + db.end_session("old", "done") + db.close() + transcripts.mkdir(parents=True, exist_ok=True) + (transcripts / "old.jsonl").write_text("{}\n", encoding="utf-8") + + set_multiplex_active(True) + try: + _run_60_ticks(SimpleNamespace(config=SimpleNamespace( + multiplex_profiles=True, sessions_dir=override))) + finally: + set_multiplex_active(False) + + assert not (override / "old.jsonl").exists(), "launch profile's configured transcript survived" + assert not (b / "sessions" / "old.jsonl").exists(), "profile b's transcript survived" + + def test_single_profile_sync_ticks_run_once_against_the_process_home(two_homes, monkeypatch): """Control: a single-profile gateway (multiplex off) still runs each chore exactly once against the process home — the named profile directory on disk is not visited.""" diff --git a/tests/hermes_state/test_auto_vacuum_in_process_holder_gate.py b/tests/hermes_state/test_auto_vacuum_in_process_holder_gate.py index 2882bf8f1b..084abfd862 100644 --- a/tests/hermes_state/test_auto_vacuum_in_process_holder_gate.py +++ b/tests/hermes_state/test_auto_vacuum_in_process_holder_gate.py @@ -1,9 +1,17 @@ -"""Automatic VACUUM is refused while THIS process holds another generation of the same store. +"""Automatic VACUUM is refused under a LIVE in-process holder — and never under a retired one. -``foreign_state_db_holders`` skips ``os.getpid()``, so it can only ever prove other PROCESSES -are away. Under the one-process-per-host multiplexer the dangerous holder is usually a sibling -SessionDB generation inside this very process: the VACUUM's TRUNCATE checkpoint retires the WAL -generation that handle is still bound to. +``foreign_state_db_holders`` skips ``os.getpid()``, so it can only ever prove other PROCESSES are +away. Under the one-process-per-host multiplexer the dangerous holder is a sibling SessionDB in +this very process: the VACUUM's TRUNCATE checkpoint retires the WAL generation that handle is +still bound to. + +The mirror invariant matters just as much. A RETIRED generation (its file was replaced by a +recovery swap, backup restore or snapshot) is already write-fenced with ``StateDbReplacedError`` +and its close-time checkpoint disabled, so it is not the writer this gate protects — and it leaves +the registry only when its holder releases, which a gateway handle does not do before shutdown. +Counting it made ONE inode replacement disable auto-VACUUM for that path for the rest of the +process lifetime, turning the unbounded growth this maintenance exists to bound into a permanent +condition. """ from __future__ import annotations @@ -31,37 +39,68 @@ def _auto_maintenance(db): min_vacuum_freelist_ratio=-1.0) -def test_auto_vacuum_skips_while_this_process_holds_another_generation(tmp_path): +def _make_prunable(db, session_id): + db.set_meta("last_auto_prune", "0") + db.create_session(session_id, "cli") + db.end_session(session_id, "done") + + +def test_auto_vacuum_skips_while_a_live_sibling_holds_the_same_store(tmp_path): + """Invariant: a genuinely LIVE sibling SessionDB for this path defers the VACUUM.""" + from hermes_state import SessionDB + db_path = tmp_path / "state.db" _seed(db_path) - first = registry.acquire(db_path) + live_sibling = registry.acquire(db_path) # e.g. the gateway's own handle + maintainer = SessionDB(db_path=db_path) # a second handle running housekeeping try: - # Snapshot restore / recovery swap shape: the file is replaced, so the next acquire - # RETIRES `first` (still open, still held) and opens a fresh generation. + _make_prunable(maintainer, "old2") + result = _auto_maintenance(maintainer) + assert result["pruned"] >= 1, result + assert result["vacuumed"] is False, "VACUUM ran under a live in-process holder" + assert result.get("vacuum_skipped_holders"), result + finally: + maintainer.close() + registry.release(live_sibling) + + # Control: the same call VACUUMs once that live sibling is gone. + quiet = SessionDB(db_path=db_path) + try: + _make_prunable(quiet, "old3") + again = _auto_maintenance(quiet) + assert again["vacuumed"] is True, again + assert "vacuum_skipped_holders" not in again, again + finally: + quiet.close() + + +def test_auto_vacuum_is_not_starved_by_a_retired_write_fenced_generation(tmp_path): + """RED on base: one inode replacement stopped auto-VACUUM for the process lifetime. + + The retired handle is never released (a gateway holds its SessionDB until shutdown), so on base + every later round reported ``vacuum_skipped_holders`` and the store grew without bound. + """ + db_path = tmp_path / "state.db" + _seed(db_path) + + parked = registry.acquire(db_path) + try: + # Recovery swap / backup restore shape: the file is replaced, so the next acquire RETIRES + # ``parked`` (still open, still held, already write-fenced) and opens a fresh generation. replacement = tmp_path / "replacement.db" shutil.copy2(db_path, replacement) replacement.replace(db_path) - second = registry.acquire(db_path) + current = registry.acquire(db_path) try: - assert second is not first, "inode replacement did not mint a new generation" - result = _auto_maintenance(second) - assert result["pruned"] == 1 - assert result["vacuumed"] is False, "VACUUM ran under a live in-process holder" - assert result.get("vacuum_skipped_holders") + assert current is not parked, "inode replacement did not mint a new generation" + for round_index in range(3): # consecutive rounds, fresh prunable rows each time + _make_prunable(current, f"old-{round_index}") + result = _auto_maintenance(current) + assert result["pruned"] >= 1, (round_index, result) + assert result["vacuumed"] is True, (round_index, result) + assert "vacuum_skipped_holders" not in result, (round_index, result) finally: - registry.release(second) + registry.release(current) finally: - registry.release(first) - - # Control: the store is quiet once the other generation is released, so the same call VACUUMs. - only = registry.acquire(db_path) - try: - only.set_meta("last_auto_prune", "0") - only.create_session("old2", "cli") - only.end_session("old2", "done") - again = _auto_maintenance(only) - assert again["vacuumed"] is True - assert "vacuum_skipped_holders" not in again - finally: - registry.release(only) + registry.release(parked) diff --git a/tests/hermes_state/test_fts_holder_instance_scope.py b/tests/hermes_state/test_fts_holder_instance_scope.py index 193aa0ffae..69cfd5314d 100644 --- a/tests/hermes_state/test_fts_holder_instance_scope.py +++ b/tests/hermes_state/test_fts_holder_instance_scope.py @@ -112,7 +112,9 @@ class TestUninspectableHolderInstanceScope: def test_other_instance_argv_is_not_a_holder_of_our_db(self, tmp_path, monkeypatch): """RED: fd dir unreadable + argv proves the process belongs to a DIFFERENT Hermes home → not a holder of our state.db.""" - db_path = tmp_path / "state.db" + # A real ``.hermes`` home, as on the field host this test is drawn from: the install + # location can only identify a home that is itself part of an install layout. + db_path = tmp_path / ".hermes" / "state.db" _install_fake_proc(monkeypatch, tmp_path, unreadable_pids=(222,)) _install_fake_argv(monkeypatch, {222: DEMO_HOME_ARGV}) @@ -155,3 +157,84 @@ class TestUninspectableHolderInstanceScope: _install_fake_proc(monkeypatch, db_path.parent, unreadable_pids=(222,)) _install_fake_argv(monkeypatch, {222: other_install}) assert hermes_state_holders.foreign_state_db_holders(db_path) == [] + + def test_shared_binary_plus_another_profile_selection_is_dismissed(self, tmp_path, monkeypatch): + """argv[0] is the SHARED install binary, so it cannot prove a hold of profile b's store. + + Every hermes process on a normal host runs ``/venv/bin/hermes``; counting that token + as proof made ``hermes -p other chat -q`` an uninspectable holder of every OTHER profile's + state.db, deferring its FTS rebuild and auto-VACUUM for as long as the sibling lived + (#92401, inside a single install). The process's own ``-p``/``--profile`` selection decides. + """ + root = tmp_path / ".hermes" + db_path = root / "profiles" / "b" / "state.db" + db_path.parent.mkdir(parents=True) + shared_binary = str(root / "venv" / "bin" / "hermes") + + for argv, expected in ( + ([shared_binary, "-p", "other", "chat", "-q"], []), + ([shared_binary, "--profile=other", "gateway", "run"], []), + # Control: the sibling that DOES serve profile b is still a fail-closed holder. + ([shared_binary, "-p", "b", "gateway", "run"], [222]), + # Control: no selection at all stays fail-closed (it may be the multiplexer). + ([shared_binary, "gateway", "run"], [222]), + ): + _install_fake_proc(monkeypatch, db_path.parent, unreadable_pids=(222,)) + _install_fake_argv(monkeypatch, {222: argv}) + holders = hermes_state_holders.foreign_state_db_holders(db_path) + assert [pid for pid, _ in holders] == expected, argv + + def test_token_naming_another_profiles_store_is_dismissal_evidence(self, tmp_path, monkeypatch): + """A token positively naming ANOTHER profile's store is the strongest dismissal there is. + + It sits under our install root, so a blanket own-prefix test read the strongest evidence of + a different scope as proof of ours. + """ + root = tmp_path / ".hermes" + db_path = root / "profiles" / "b" / "state.db" + db_path.parent.mkdir(parents=True) + other_store = root / "profiles" / "other" / "state.db" + + for argv in ( + ["hermes", f"--db={other_store}", "sessions", "optimize"], + [str(root / "venv" / "bin" / "hermes"), "sessions", str(other_store)], + ): + _install_fake_proc(monkeypatch, db_path.parent, unreadable_pids=(222,)) + _install_fake_argv(monkeypatch, {222: argv}) + assert hermes_state_holders.foreign_state_db_holders(db_path) == [], argv + + def test_unrelated_install_under_a_non_hermes_profiles_tree_stays_dismissed( + self, tmp_path, monkeypatch + ): + """``/profiles//state.db`` does not make all of ```` ours. + + A raw ``basename == "profiles"`` test promoted any such parent to the install root, so an + unrelated Hermes install living under it was counted as a holder — the literal two-instance + shape #92401 was filed about. The canonical ``named_profile_home`` predicate requires the + parent to be a real Hermes home. + """ + work = tmp_path / "work" + db_path = work / "profiles" / "b" / "state.db" + db_path.parent.mkdir(parents=True) + unrelated_home = work / "demo" / ".hermes" + unrelated_home.mkdir(parents=True) + + _install_fake_proc(monkeypatch, db_path.parent, unreadable_pids=(222,)) + _install_fake_argv( + monkeypatch, {222: ["hermes", "--hermes-home", str(unrelated_home), "gateway", "run"]}) + assert hermes_state_holders.foreign_state_db_holders(db_path) == [] + + def test_custom_home_is_not_dismissed_by_the_install_location(self, tmp_path, monkeypatch): + """A store at a custom HERMES_HOME is SERVED BY the binary under ``~/.hermes``. + + Dismissing on that argv[0] admitted auto-VACUUM and the FTS rebuild under the live + multiplexer that holds the store. argv[0] locates the install, never the home. + """ + db_path = tmp_path / "custom-store" / "state.db" + db_path.parent.mkdir(parents=True) + + _install_fake_proc(monkeypatch, db_path.parent, unreadable_pids=(222,)) + _install_fake_argv( + monkeypatch, {222: ["/home/u/.hermes/venv/bin/hermes", "gateway", "run"]}) + holders = hermes_state_holders.foreign_state_db_holders(db_path) + assert [pid for pid, _ in holders] == [222] diff --git a/tests/hermes_state/test_remove_session_files_glob_escape.py b/tests/hermes_state/test_remove_session_files_glob_escape.py new file mode 100644 index 0000000000..898b1bfccf --- /dev/null +++ b/tests/hermes_state/test_remove_session_files_glob_escape.py @@ -0,0 +1,23 @@ +"""Pruned-session file removal is id-scoped even when the id carries glob metacharacters. + +``request_dump__*.json`` was interpolated unescaped, so an id containing ``[``/``?``/``*`` was +a PATTERN: its own dumps were left behind and another session's could be matched instead. +""" + +from __future__ import annotations + +from hermes_state_sessions import SessionSessionsMixin + + +def test_remove_session_files_escapes_glob_metacharacters(tmp_path): + tricky, neighbour = "sess-[ab]-1", "sess-a-1" + for session_id in (tricky, neighbour): + (tmp_path / f"{session_id}.jsonl").write_text("{}\n", encoding="utf-8") + (tmp_path / f"request_dump_{session_id}_0.json").write_text("{}", encoding="utf-8") + + SessionSessionsMixin._remove_session_files(tmp_path, tricky) + + assert not (tmp_path / f"{tricky}.jsonl").exists() + assert not (tmp_path / f"request_dump_{tricky}_0.json").exists(), "own dump was not matched" + assert (tmp_path / f"{neighbour}.jsonl").exists() + assert (tmp_path / f"request_dump_{neighbour}_0.json").exists(), "neighbour's dump was removed"