From af595ff8a68bb75702f6d779cd236d62009dfaa1 Mon Sep 17 00:00:00 2001 From: Benjamin PERRY Date: Thu, 24 Sep 2026 13:16:13 +0000 Subject: [PATCH] fix(files): preserve SQLite locks during previews Co-Authored-By: Hermes Agent / OpenAI Codex / gpt-6-sol (cherry picked from commit 60d879c125177cd4cacdbc840f16c2e048b4199c) --- agent/context_references.py | 35 +++++++-- hermes_cli/sqlite_safe_read.py | 10 ++- hermes_cli/web_routers/files.py | 13 +++- tests/agent/test_live_db_preview_locks.py | 91 +++++++++++++++++++++++ 4 files changed, 136 insertions(+), 13 deletions(-) create mode 100644 tests/agent/test_live_db_preview_locks.py diff --git a/agent/context_references.py b/agent/context_references.py index 8677eb44d2..0f84216276 100644 --- a/agent/context_references.py +++ b/agent/context_references.py @@ -296,6 +296,21 @@ def _expand_path_reference(ref: ContextReference, cwd: Path, *, allowed_root: Pa if is_folder: listing = _build_folder_listing(path, cwd, display_base=allowed_root) return None, f"📁 {ref.raw} ({estimate_tokens_rough(listing)} tokens)\n{listing}" + from hermes_cli.sqlite_safe_read import LiveConnectionError, offline_file_access + try: + # Keep admission through every sniff, line count and text read: a connection + # can start between a check and a later open otherwise. + with offline_file_access(path, what="preview context reference"): + return _expand_file_reference(ref, path, max_inline_tokens) + except LiveConnectionError: + return None, _on_disk_reference_block( + ref, path, descriptor="live SQLite database file", + reason="not previewed: raw access would cancel SQLite's POSIX locks.", + guidance="Do not open this file directly while its database connection is live.", + ) + + +def _expand_file_reference(ref: ContextReference, path: Path, max_inline_tokens: int | None) -> Expansion: if _is_binary_file(path): # A bare "not supported" warning was a dead end (the model gave up); the file IS # on disk where the agent's tools run, so hand it an actionable block instead. @@ -659,13 +674,17 @@ def _file_metadata(path: Path) -> str: return "unknown size" # A listing line is a summary, not content: past the cap, byte size conveys the # same "how big is this" without a full scan per entry. - if _is_binary_file(path) or size > _LINE_COUNT_MAX_BYTES: - return f"{size} bytes" + from hermes_cli.sqlite_safe_read import LiveConnectionError, offline_file_access try: - with path.open("rb") as fh: - # UTF-8 never embeds 0x0A inside a multibyte sequence, so counting bytes - # matches a decoded newline count while streaming instead of read_text. - lines = sum(chunk.count(b"\n") for chunk in iter(lambda: fh.read(1 << 20), b"")) - return f"{lines + 1} lines" - except Exception: + # A directory preview inspects each entry separately; the registry lock + # must cover both its binary sniff and optional line-count read. + with offline_file_access(path, what="inspect folder entry"): + if _is_binary_file(path) or size > _LINE_COUNT_MAX_BYTES: + return f"{size} bytes" + with path.open("rb") as fh: + # UTF-8 never embeds 0x0A inside a multibyte sequence, so counting bytes + # matches a decoded newline while streaming instead of read_text. + lines = sum(chunk.count(b"\n") for chunk in iter(lambda: fh.read(1 << 20), b"")) + return f"{lines + 1} lines" + except (LiveConnectionError, OSError): return f"{size} bytes" diff --git a/hermes_cli/sqlite_safe_read.py b/hermes_cli/sqlite_safe_read.py index 154e43f31c..c817ab1cda 100644 --- a/hermes_cli/sqlite_safe_read.py +++ b/hermes_cli/sqlite_safe_read.py @@ -230,7 +230,15 @@ def offline_file_access(path: Path | str, *, what: str = "read"): :func:`has_live_connection` and *then* doing raw I/O is a check/use race (a connection opened in between loses its POSIX locks to the raw ``close()``). Held only for the raw I/O.""" with _live_lock: - if _key(path) in _live_connections: + key = _key(path) + # SQLite locks the main file and its WAL shared-memory sidecar. A raw + # close of either inode cancels this process's POSIX locks, while the + # connection registry is keyed by the main database path. + live = key in _live_connections or any( + key.endswith(suffix) and key[:-len(suffix)] in _live_connections + for suffix in ("-wal", "-shm") + ) + if live: raise LiveConnectionError( f"Refusing to {what} {path}: a connection to it is still open " "in this process, and raw file access would cancel that " diff --git a/hermes_cli/web_routers/files.py b/hermes_cli/web_routers/files.py index 54591d684a..fb1a9d0287 100644 --- a/hermes_cli/web_routers/files.py +++ b/hermes_cli/web_routers/files.py @@ -176,11 +176,16 @@ def _fs_regular_file(path: Path) -> tuple[Path, os.stat_result]: def _fs_read_bytes(target: Path, limit: Optional[int] = None) -> bytes: """Read (a prefix of) ``target``; 403/400 on failure.""" + from hermes_cli.sqlite_safe_read import LiveConnectionError, offline_file_access try: - if limit is None: - return target.read_bytes() - with target.open("rb") as handle: - return handle.read(limit) + # Keep admission through close; a raw close cancels this process's SQLite locks. + with offline_file_access(target, what="preview file"): + if limit is None: + return target.read_bytes() + with target.open("rb") as handle: + return handle.read(limit) + except LiveConnectionError as exc: + raise HTTPException(status_code=409, detail=str(exc)) from exc except PermissionError: raise HTTPException(status_code=403, detail="File is not readable") except OSError as exc: diff --git a/tests/agent/test_live_db_preview_locks.py b/tests/agent/test_live_db_preview_locks.py new file mode 100644 index 0000000000..6dbb9669d7 --- /dev/null +++ b/tests/agent/test_live_db_preview_locks.py @@ -0,0 +1,91 @@ +"""File previews may not cancel SQLite's live POSIX locks (including WAL sidecars).""" +import asyncio +import os +from pathlib import Path +import sqlite3 +import subprocess +import sys + +import pytest +from fastapi import HTTPException + +from agent.context_references import _expand_path_reference, parse_context_references +from hermes_state import SessionDB +from hermes_cli.web_routers.files import fs_read_text + + +@pytest.mark.linux_only +@pytest.mark.parametrize("route,target_kind", [ + ("file", "main"), ("file", "shm"), ("file", "shm_alias"), ("file", "wal"), + ("folder", "directory"), ("desktop", "main"), + ("desktop", "shm"), ("desktop", "shm_alias"), ("desktop", "wal"), +]) +def test_preview_preserves_live_database_locks(tmp_path, route, target_kind): + path = tmp_path / "state.db" + text = tmp_path / "normal.txt" + text.write_text("ordinary readable text", encoding="utf-8") + db = SessionDB(path) + try: + db.create_session("preview-test", "cli") + db.append_message("preview-test", "user", "before preview") + shm = Path(str(path) + "-shm") + wal = Path(str(path) + "-wal") + alias = tmp_path / "linked-shm" + alias.symlink_to(shm) + target = {"main": path, "shm": shm, "shm_alias": alias, + "wal": wal, "directory": tmp_path}[target_kind] + conn = db._conn + assert isinstance(conn, sqlite3.Connection) + conn.execute("BEGIN IMMEDIATE") + + def posix_locks(file): + inode = file.stat().st_ino + return sorted(line.split(": ", 1)[1] for line in Path("/proc/locks").read_text(encoding="utf-8").splitlines() + if f":{inode} " in line and f"POSIX ADVISORY" in line + and f" {os.getpid()} " in line) + + def rival_locked(): + code = ("import sqlite3,sys; c=sqlite3.connect(sys.argv[1], timeout=0); " + "c.execute('BEGIN IMMEDIATE'); c.rollback(); c.close()") + result = subprocess.run([sys.executable, "-c", code, str(path)], + capture_output=True, text=True, timeout=10) + return result.returncode != 0 and "database is locked" in result.stderr + + before = (posix_locks(path), posix_locks(shm)) + assert all(before), "fixture must hold POSIX main and WAL-sidecar locks" + assert rival_locked(), "second process must be excluded before the preview" + + if route == "desktop": + with pytest.raises(HTTPException) as refused: + asyncio.run(fs_read_text(str(target))) + assert refused.value.status_code == 409 + assert asyncio.run(fs_read_text(str(text)))["text"] == "ordinary readable text" + else: + ref = parse_context_references(f"@{route}:{target}")[0] + warning, block = _expand_path_reference(ref, tmp_path.parent) + assert warning is None + assert block is not None + assert "not previewed" in block if route == "file" else "state.db" in block + ordinary = parse_context_references(f"@file:{text}")[0] + warning, block = _expand_path_reference(ordinary, tmp_path.parent) + assert warning is None and block is not None and "ordinary readable text" in block + + assert (posix_locks(path), posix_locks(shm)) == before + assert rival_locked(), "second process entered a still-open write transaction" + conn.rollback() + db.append_message("preview-test", "assistant", "after preview") + assert len(db.get_messages("preview-test")) == 2 + finally: + db.close() + + +def test_closed_database_can_still_be_previewed(tmp_path): + path = tmp_path / "offline.db" + db = SessionDB(path) + db.create_session("offline", "cli") + db.close() + ref = parse_context_references(f"@file:{path}")[0] + warning, block = _expand_path_reference(ref, tmp_path.parent) + assert warning is None and block is not None and "binary file" in block + preview = asyncio.run(fs_read_text(str(path))) + assert preview["binary"] is True and preview["byteSize"] == path.stat().st_size