From 48196b0ceb4bddb5553d3d305f0efc74ba878394 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sun, 27 Sep 2026 15:46:36 +0530 Subject: [PATCH] fix(files): refuse live-DB downloads and share one sidecar-aware liveness check FileResponse opens and closes the file in the dashboard process, so downloading a live state.db (or its -shm/-wal) via /api/fs/download or the managed-file read/download/media routes still cancelled the connection's POSIX locks. Both now return 409 via is_live_database_file; the registry lock is not held across the streamed response. The main-or-WAL-sidecar rule now lives in one _live_main_key helper used by offline_file_access, has_live_connection and read_header_bytes_preopen, and the sidecar refusal names the main database the connection is open on. @file previews hold _live_lock only for the raw read; token counting and formatting run after release. The Linux lock test gains requires_wal (Hermes uses DELETE mode on WAL-reset-vulnerable SQLite), covers the download refusal, and drops an ambiguous conditional assert. Co-authored-by: Benjamin PERRY --- agent/context_references.py | 22 +++++++----- hermes_cli/sqlite_safe_read.py | 41 ++++++++++++++++------- hermes_cli/web_routers/files.py | 20 +++++++++-- tests/agent/test_live_db_preview_locks.py | 15 +++++++-- 4 files changed, 72 insertions(+), 26 deletions(-) diff --git a/agent/context_references.py b/agent/context_references.py index 0f84216276..c44c5327ef 100644 --- a/agent/context_references.py +++ b/agent/context_references.py @@ -17,6 +17,7 @@ from typing import Awaitable, Callable from agent.model_metadata import CHARS_PER_TOKEN, estimate_tokens_rough from hermes_cli._subprocess_compat import IS_WINDOWS, harden_git_argv, noninteractive_git_env, windows_hide_flags +from hermes_cli.sqlite_safe_read import LiveConnectionError, offline_file_access from hermes_cli.sizefmt import format_bytes # ── Plugin context-reference provider API ──────────────────────────────────── @@ -296,21 +297,23 @@ 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. + # Keep admission through every sniff, stat and text read (a connection can start + # between a check and a later open otherwise), but release it before token + # counting/formatting: the registry lock blocks every tracked connect/close. with offline_file_access(path, what="preview context reference"): - return _expand_file_reference(ref, path, max_inline_tokens) + raw = _read_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.", ) + return raw if isinstance(raw, tuple) else _format_file_reference(ref, path, raw, max_inline_tokens) -def _expand_file_reference(ref: ContextReference, path: Path, max_inline_tokens: int | None) -> Expansion: +def _read_file_reference(ref: ContextReference, path: Path, max_inline_tokens: int | None) -> str | Expansion: + """Raw file I/O for an @file ref: the text to inline, or an early refusal block.""" 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. @@ -363,6 +366,10 @@ def _expand_file_reference(ref: ContextReference, path: Path, max_inline_tokens: if max_inline_tokens is not None and size > max_inline_tokens * CHARS_PER_TOKEN: return None, _oversized_text_reference_block(ref, path, size // CHARS_PER_TOKEN) text = path.read_text(encoding="utf-8-sig") + return text + + +def _format_file_reference(ref: ContextReference, path: Path, text: str, max_inline_tokens: int | None) -> Expansion: lang = _FENCE_LANGUAGES.get(path.suffix.lower(), "") text_tokens = estimate_tokens_rough(text) # Check BEFORE building the fenced block: an oversized file is not going to be @@ -672,13 +679,12 @@ def _file_metadata(path: Path) -> str: size = path.stat().st_size except OSError: 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. - from hermes_cli.sqlite_safe_read import LiveConnectionError, offline_file_access try: # 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"): + # 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" with path.open("rb") as fh: diff --git a/hermes_cli/sqlite_safe_read.py b/hermes_cli/sqlite_safe_read.py index c817ab1cda..3cfaaf443a 100644 --- a/hermes_cli/sqlite_safe_read.py +++ b/hermes_cli/sqlite_safe_read.py @@ -76,10 +76,31 @@ def untrack_connection(path: Path | str) -> None: _track_key(_key(path), -1) +def _live_main_key(key: str) -> Optional[str]: + """The tracked main-database key that makes *key* live, or ``None`` (caller holds ``_live_lock``). + + SQLite locks the main file and its WAL sidecars; a raw ``close()`` of any of those + inodes cancels this process's POSIX locks, but the registry is keyed by the main path.""" + if key in _live_connections: + return key + for suffix in ("-wal", "-shm"): + if key.endswith(suffix) and key[:-len(suffix)] in _live_connections: + return key[:-len(suffix)] + return None + + def has_live_connection(path: Path | str) -> bool: - """Whether this process currently holds any connection to *path*.""" + """Whether this process holds a connection to *path* (or to the database it is a sidecar of).""" with _live_lock: - return _key(path) in _live_connections + return _live_main_key(_key(path)) is not None + + +def is_live_database_file(path: Path | str) -> bool: + """Whether a raw open/close of *path* would cancel a live connection's POSIX locks. + + Point-in-time answer for callers that cannot hold ``_live_lock`` across their I/O + (e.g. a streamed download: the lock must never be held across an await/yield).""" + return has_live_connection(path) class _TrackingMixin: @@ -211,7 +232,7 @@ def read_header_bytes_preopen(path: Path | str, *, length: int = 100, force: boo overwritten?). Check and open/read/close run together under ``_live_lock`` so a connection cannot be opened between deciding "nothing is live" and closing this descriptor.""" with _live_lock: - if not force and _key(path) in _live_connections: + if not force and _live_main_key(_key(path)) is not None: logger.debug( "refusing byte-level read of %s: a live connection exists in " "this process and close() would cancel its POSIX locks", @@ -230,17 +251,11 @@ 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: - 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: + main = _live_main_key(_key(path)) + if main is not None: + subject = "it" if main == _key(path) else f"its main database {main}" raise LiveConnectionError( - f"Refusing to {what} {path}: a connection to it is still open " + f"Refusing to {what} {path}: a connection to {subject} is still open " "in this process, and raw file access would cancel that " "connection's POSIX advisory locks. Close all database " "handles (stop the gateway/dashboard) and retry.") diff --git a/hermes_cli/web_routers/files.py b/hermes_cli/web_routers/files.py index fb1a9d0287..8581edbb75 100644 --- a/hermes_cli/web_routers/files.py +++ b/hermes_cli/web_routers/files.py @@ -25,6 +25,7 @@ from fastapi import APIRouter, File, Form, HTTPException, Request, UploadFile from fastapi.responses import FileResponse from hermes_cli._subprocess_compat import windows_hide_flags +from hermes_cli.sqlite_safe_read import LiveConnectionError, is_live_database_file, offline_file_access from hermes_cli.web_deps import late from hermes_cli.web_server_files import ( _fs_path, _managed_file_entry, _managed_response_meta, _resolve_managed_path, @@ -174,9 +175,22 @@ def _fs_regular_file(path: Path) -> tuple[Path, os.stat_result]: return target, st +def _refuse_live_database(target: Path) -> None: + """409 before streaming ``target`` while this process has a live SQLite connection to it. + + ``FileResponse`` opens and closes the file in-process, and that raw close cancels the + connection's POSIX locks. The registry lock can't be held across a streamed response + (never across an await/yield), so this is a point-in-time admission check.""" + if is_live_database_file(target): + raise HTTPException( + status_code=409, + detail=f"Refusing to download {target}: a SQLite connection to its database is " + "open in this process, and raw file access would cancel its POSIX advisory locks.", + ) + + 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 + """Read (a prefix of) ``target``; 403/400 on failure, 409 while a SQLite connection to it is live.""" try: # Keep admission through close; a raw close cancels this process's SQLite locks. with offline_file_access(target, what="preview file"): @@ -404,6 +418,7 @@ def _managed_readable_file(request: Request, path: str) -> tuple[Any, Path, str, raise HTTPException(status_code=400, detail="Path is not a file") if _is_sensitive_path(target): raise HTTPException(status_code=403, detail="Access to sensitive files is not allowed") + _refuse_live_database(target) mime_type = mimetypes.guess_type(target.name)[0] or "application/octet-stream" return policy, target, display_path, _MANAGED_FILE_MAX_BYTES, mime_type @@ -740,6 +755,7 @@ async def fs_download( path: str, profile: Optional[str] = None, session_id: Optional[str] = None, ): target, _st = _fs_regular_file(await _fs_download_path(path, profile, session_id)) + _refuse_live_database(target) return FileResponse( path=str(target), media_type=_fs_mime_type(target), diff --git a/tests/agent/test_live_db_preview_locks.py b/tests/agent/test_live_db_preview_locks.py index 01e158d955..f0da0eb1a9 100644 --- a/tests/agent/test_live_db_preview_locks.py +++ b/tests/agent/test_live_db_preview_locks.py @@ -11,10 +11,11 @@ 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 +from hermes_cli.web_routers.files import fs_download, fs_read_text @pytest.mark.platforms("linux") +@pytest.mark.requires_wal @pytest.mark.parametrize("route,target_kind", [ ("file", "main"), ("folder", "directory"), ("desktop", "main"), ("desktop", "shm"), ]) @@ -45,7 +46,7 @@ def test_preview_preserves_live_database_locks(tmp_path, route, target_kind): 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 + if f":{inode} " in line and "POSIX ADVISORY" in line and f" {os.getpid()} " in line) def rival_locked(): @@ -63,13 +64,20 @@ def test_preview_preserves_live_database_locks(tmp_path, route, target_kind): with pytest.raises(HTTPException) as refused: asyncio.run(fs_read_text(str(target))) assert refused.value.status_code == 409 + if target_kind == "shm": + assert "main database" in refused.value.detail + # FileResponse opens/closes in-process too, so a download must be refused as well. + with pytest.raises(HTTPException) as refused: + asyncio.run(fs_download(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 + if route == "file": + assert "not previewed" 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 @@ -107,3 +115,4 @@ def test_closed_database_can_still_be_previewed(tmp_path): 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 + assert asyncio.run(fs_download(str(path))).path == str(path)