fix(secret_scope): memoise the shared .env tokenizer once per file change
load_env_file() is now the canonical .env tokenizer afterc849bc383acollapsed six hand parsers onto it. Profile scopes, dashboard boundary discovery, skill secret capture, managed .env and setup readers share the same decoding and parsing rules, but direct calls still re-read and re-parse the whole file. build_profile_secret_scope() puts that work on the gateway's hottest paths: every turn, every cron job, MCP and browser lifecycle adoption, the TUI prompt turn, and the 60s housekeeping drain of the durable cron delivery queue. Memoising the shared tokenizer now removes redundant work for the consolidated readers as well as the motivating profile-scope path. config.load_env() keeps its existing outer memo over a call that is now itself memoised; this leaves its menu-render shortcut intact without redesigning it. Freshness is the hard part, because a stale credential map is a far worse failure than a slow one. Every load_env_file() call still OPENS the file and keys on os.fstat() of that descriptor rather than os.stat() of the path: * the open preserves close-to-open revalidation on NFS; * an open or read failure returns {} without caching it, and drops any warm entry so a transient EACCES cannot become a persistent empty profile; * the descriptor pins one inode, so a symlink repointed mid-read cannot file one file's contents under another file's identity. The key is (mtime_ns, size, inode, device), re-checked on the same descriptor after reading. A rewrite through that inode is not stored under the pre-read fingerprint. Unavailable metadata means "do not cache", never "cannot read". A generation counter checked under the lock prevents a slow reader from repopulating an entry that a writer invalidated while the read was in flight. Read bytes from the open descriptor and preserve upstream's decoding exactly: strip a UTF-8 BOM before trying UTF-8, then fall back to latin-1 for invalid UTF-8. Both the cached reader and the uncached reference use that decoder and the shared text parser. Invalid UTF-8 is a cacheable result, not a read error. The value and inline-comment helpers remain local to secret_scope. invalidate_env_cache() clears this memo too, so save_env_value, remove_env_value and sanitize_env_file invalidate both layers. The per-path LRU holds at most 64 entries and every caller gets its own dict. Accepted limitation: a write keeping length, inode and nanosecond mtime all identical is not detected without explicit invalidation. This is a new limitation for previously uncached readers. Parsed secrets also remain reachable in memory between use and eviction for up to 64 paths, where the outer config memo holds one. The pre-rebase measurement on a 508-line, 25432-byte .env was 0.868 ms -> 0.011 ms per call. That measurement predates the shared binary decoder. Add decode/cache invariants for BOM and plain files, latin-1 fallback, same-size writes switching UTF-8 -> latin-1 -> UTF-8, and agreement with the uncached reference. All eight decoding, cache and freshness mutations fail the new tests, and all 40 cache tests pass after restoration. The requested agent, CLI, cron and gateway suites report 30,168 passed, 479 failed and 303 skipped in the restricted local environment. All 140 failing files were checked against upstream/main at5eb99eb284: all 479 failed-test IDs and 210 collection/setup/teardown error IDs match exactly.
This commit is contained in:
committed by
kshitij
parent
4da4382ca0
commit
0d6fec59d4
@@ -14,9 +14,11 @@ from __future__ import annotations
|
||||
import codecs
|
||||
import os
|
||||
import re
|
||||
import threading
|
||||
from collections import OrderedDict
|
||||
from contextvars import ContextVar, Token
|
||||
from pathlib import Path
|
||||
from typing import Dict, Mapping, Optional
|
||||
from typing import Dict, Mapping, Optional, Tuple
|
||||
|
||||
|
||||
# Process-global (describes the deployment mode, not a per-task value): set once
|
||||
@@ -219,27 +221,52 @@ def _parse_env_value(raw_value: str) -> str:
|
||||
return value
|
||||
|
||||
|
||||
def load_env_file(env_path: Path) -> Dict[str, str]:
|
||||
"""THE ``.env`` tokenizer: every reader (profile scope, ``hermes_cli.config.load_env``, the dashboard
|
||||
scrub, skill secret capture, managed .env, setup prompts) parses through here so no two boundaries
|
||||
disagree on which keys/values a file defines. Dict only — never touches ``os.environ``. ``export``
|
||||
prefix, ``#`` comments, quote escapes reversed; ``utf-8-sig`` so a BOM doesn't prefix the first key.
|
||||
Invalid UTF-8 decodes as latin-1, exactly like ``env_loader._load_dotenv_with_fallback`` installs it
|
||||
into ``os.environ``. Absent/unreadable → ``{}``."""
|
||||
secrets: Dict[str, str] = {}
|
||||
try:
|
||||
raw = env_path.read_bytes()
|
||||
except OSError:
|
||||
return secrets
|
||||
# Per-path memo of parsed ``.env`` files. ``build_profile_secret_scope()`` runs on every gateway
|
||||
# turn, cron fire, MCP/browser adoption and housekeeping drain, and each call used to re-read and
|
||||
# re-parse the whole file.
|
||||
#
|
||||
# FRESHNESS: every call still OPENS the file and keys on the ``fstat`` of that descriptor
|
||||
# (mtime_ns, size, inode, device), re-checked after the read. The open keeps close-to-open
|
||||
# revalidation on NFS, a vanished/unreadable file fails the open and is never cached (a transient
|
||||
# EACCES must not become "this profile has no secrets"), and the descriptor pins one inode so a
|
||||
# symlink repointed mid-read can't file one file's contents under another's identity. Accepted gap: a
|
||||
# rewrite keeping length, inode AND nanosecond mtime identical; ``invalidate_env_file_cache()`` is the
|
||||
# knob, and ``hermes_cli.config.invalidate_env_cache()`` calls it for Hermes's own .env writers.
|
||||
_ENV_FILE_CACHE: "OrderedDict[str, Tuple[tuple, Dict[str, str]]]" = OrderedDict()
|
||||
_ENV_FILE_CACHE_LOCK = threading.Lock()
|
||||
_ENV_FILE_CACHE_MAX = 64 # one entry per profile home in practice
|
||||
|
||||
|
||||
def _fd_fingerprint(fileno: int) -> tuple:
|
||||
st = os.fstat(fileno)
|
||||
return (st.st_mtime_ns, st.st_size, st.st_ino, st.st_dev)
|
||||
|
||||
|
||||
def invalidate_env_file_cache(env_path: Optional[Path] = None) -> None:
|
||||
"""Drop one path from the ``load_env_file()`` memo, or all of them."""
|
||||
with _ENV_FILE_CACHE_LOCK:
|
||||
if env_path is None:
|
||||
_ENV_FILE_CACHE.clear()
|
||||
else:
|
||||
_ENV_FILE_CACHE.pop(str(env_path), None)
|
||||
|
||||
|
||||
def _decode_env_bytes(raw: bytes) -> str:
|
||||
"""BOM stripped; invalid UTF-8 falls back to latin-1 exactly as
|
||||
``env_loader._load_dotenv_with_fallback`` installs it into ``os.environ``."""
|
||||
if raw.startswith(codecs.BOM_UTF8):
|
||||
raw = raw[len(codecs.BOM_UTF8):]
|
||||
try:
|
||||
text = raw.decode("utf-8")
|
||||
return raw.decode("utf-8")
|
||||
except UnicodeDecodeError:
|
||||
text = raw.decode("latin-1")
|
||||
return raw.decode("latin-1")
|
||||
|
||||
for raw in text.splitlines():
|
||||
line = raw.strip()
|
||||
|
||||
def _parse_env_text(text: str) -> Dict[str, str]:
|
||||
"""Tokenize already-read ``.env`` text. See :func:`load_env_file`."""
|
||||
secrets: Dict[str, str] = {}
|
||||
for raw_line in text.splitlines():
|
||||
line = raw_line.strip()
|
||||
if not line or line.startswith("#"):
|
||||
continue
|
||||
if line.startswith("export "):
|
||||
@@ -251,6 +278,46 @@ def load_env_file(env_path: Path) -> Dict[str, str]:
|
||||
return secrets
|
||||
|
||||
|
||||
def load_env_file(env_path: Path) -> Dict[str, str]:
|
||||
"""THE ``.env`` tokenizer: every reader (profile scope, ``hermes_cli.config.load_env``, the dashboard
|
||||
scrub, skill secret capture, managed .env, setup prompts) parses through here so no two boundaries
|
||||
disagree on which keys/values a file defines. Dict only — never touches ``os.environ``. ``export``
|
||||
prefix, ``#`` comments, quote escapes reversed; a BOM is stripped so it doesn't prefix the first key.
|
||||
Invalid UTF-8 decodes as latin-1, exactly like ``env_loader._load_dotenv_with_fallback`` installs it
|
||||
into ``os.environ``. Absent/unreadable → ``{}``.
|
||||
|
||||
Memoised per path on the open descriptor's stat identity (see the cache comment above). Always
|
||||
returns a fresh dict: callers mutate what they get back (``build_profile_secret_scope`` layers
|
||||
external secrets over it).
|
||||
"""
|
||||
key = str(env_path)
|
||||
try:
|
||||
with open(env_path, "rb") as handle:
|
||||
fingerprint = _fd_fingerprint(handle.fileno())
|
||||
with _ENV_FILE_CACHE_LOCK:
|
||||
cached = _ENV_FILE_CACHE.get(key)
|
||||
if cached is not None and cached[0] == fingerprint:
|
||||
_ENV_FILE_CACHE.move_to_end(key)
|
||||
return dict(cached[1])
|
||||
raw = handle.read()
|
||||
# Same descriptor: a rewrite that landed between the fstat and the read is parsed but not
|
||||
# stored under the pre-write fingerprint.
|
||||
settled = _fd_fingerprint(handle.fileno()) == fingerprint
|
||||
except OSError:
|
||||
# Gone or unreadable: drop any entry so a stale map cannot outlive the file.
|
||||
invalidate_env_file_cache(env_path)
|
||||
return {}
|
||||
|
||||
secrets = _parse_env_text(_decode_env_bytes(raw))
|
||||
if settled:
|
||||
with _ENV_FILE_CACHE_LOCK:
|
||||
_ENV_FILE_CACHE[key] = (fingerprint, dict(secrets))
|
||||
_ENV_FILE_CACHE.move_to_end(key)
|
||||
while len(_ENV_FILE_CACHE) > _ENV_FILE_CACHE_MAX:
|
||||
_ENV_FILE_CACHE.popitem(last=False)
|
||||
return secrets
|
||||
|
||||
|
||||
def build_profile_secret_scope(hermes_home: Path) -> Dict[str, str]:
|
||||
"""Build a profile's secret mapping from ``<home>/.env`` plus its external
|
||||
secret sources. Global vars are NOT copied in — ``get_secret`` reads those
|
||||
|
||||
@@ -2456,9 +2456,14 @@ def load_env() -> Dict[str, str]:
|
||||
|
||||
|
||||
def invalidate_env_cache() -> None:
|
||||
"""Clear the load_env() memo so the next call sees a write even on coarse-mtime filesystems."""
|
||||
"""Clear the load_env() memo AND ``agent.secret_scope``'s per-path ``.env`` memo so the next call
|
||||
sees a write even on coarse-mtime filesystems; the writers that already call this
|
||||
(save_env_value / remove_env_value / sanitize_env_file) need not know about both caches."""
|
||||
from agent.secret_scope import invalidate_env_file_cache
|
||||
|
||||
global _env_cache
|
||||
_env_cache = None
|
||||
invalidate_env_file_cache()
|
||||
|
||||
|
||||
def _sanitize_env_lines(lines: list) -> list:
|
||||
|
||||
79
tests/agent/test_secret_scope_env_cache.py
Normal file
79
tests/agent/test_secret_scope_env_cache.py
Normal file
@@ -0,0 +1,79 @@
|
||||
"""The per-path ``load_env_file()`` memo in ``agent.secret_scope``.
|
||||
|
||||
``build_profile_secret_scope()`` sits on the gateway's hot paths (every turn, cron fire, MCP/browser
|
||||
adoption, housekeeping drain) and used to re-read and re-parse the profile ``.env`` on each call.
|
||||
A stale credential map is a worse failure than a slow one, so the contract under test is: parse once
|
||||
while the file is unchanged, hand every caller its own dict, and see every real change on disk.
|
||||
|
||||
Files are written as bytes: ``Path.write_text`` translates ``\\n`` to ``\\r\\n`` on Windows, which
|
||||
would break the same-size assertion.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
|
||||
import pytest
|
||||
|
||||
import agent.secret_scope as ss
|
||||
|
||||
# Comfortably past the coarsest mtime resolution in common use (FAT32 truncates to two seconds).
|
||||
_MTIME_STEP_NS = 10_000_000_000
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _clear_cache():
|
||||
ss.invalidate_env_file_cache()
|
||||
yield
|
||||
ss.invalidate_env_file_cache()
|
||||
|
||||
|
||||
def _count_parses(monkeypatch) -> list:
|
||||
calls: list = []
|
||||
real = ss._parse_env_text
|
||||
monkeypatch.setattr(ss, "_parse_env_text", lambda t: (calls.append(t), real(t))[1])
|
||||
return calls
|
||||
|
||||
|
||||
def _rewrite_same_size(path, text: str) -> None:
|
||||
"""Same length, different content: only the mtime distinguishes the versions."""
|
||||
previous = path.stat()
|
||||
path.write_bytes(text.encode("utf-8"))
|
||||
assert path.stat().st_size == previous.st_size
|
||||
bumped = previous.st_mtime_ns + _MTIME_STEP_NS
|
||||
os.utime(path, ns=(bumped, bumped))
|
||||
|
||||
|
||||
def test_unchanged_file_is_parsed_once_and_each_caller_gets_its_own_dict(tmp_path, monkeypatch):
|
||||
env = tmp_path / ".env"
|
||||
env.write_bytes(b"A=1\nB=2\n")
|
||||
calls = _count_parses(monkeypatch)
|
||||
monkeypatch.setattr("hermes_cli.env_loader.get_secret_source_values", lambda home: {"EXT": "vault"})
|
||||
|
||||
first = ss.load_env_file(env)
|
||||
first["INJECTED"] = "must-not-persist" # callers mutate what they get back
|
||||
for _ in range(5):
|
||||
assert ss.load_env_file(env) == {"A": "1", "B": "2"}
|
||||
# The motivating path layers external secrets onto the result; that must not poison the memo.
|
||||
scope = ss.build_profile_secret_scope(tmp_path)
|
||||
assert scope["EXT"] == "vault"
|
||||
assert ss.load_env_file(env) == {"A": "1", "B": "2"}
|
||||
|
||||
assert len(calls) == 1, f"expected one parse, got {len(calls)}"
|
||||
|
||||
|
||||
def test_edits_and_deletion_are_seen(tmp_path, monkeypatch):
|
||||
env = tmp_path / ".env"
|
||||
env.write_bytes(b"KEY=aaa\n")
|
||||
calls = _count_parses(monkeypatch)
|
||||
assert ss.load_env_file(env) == {"KEY": "aaa"}
|
||||
|
||||
_rewrite_same_size(env, "KEY=bbb\n")
|
||||
assert ss.load_env_file(env) == {"KEY": "bbb"}
|
||||
assert len(calls) == 2
|
||||
|
||||
env.unlink()
|
||||
assert ss.load_env_file(env) == {}
|
||||
env.write_bytes(b"KEY=ccc\n")
|
||||
assert ss.load_env_file(env) == {"KEY": "ccc"}
|
||||
assert len(calls) == 3
|
||||
Reference in New Issue
Block a user