From fc53717f723e53dc22b192e2f82a6d1ebd97cd49 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sun, 27 Sep 2026 16:33:48 +0530 Subject: [PATCH] fix(files): one live-DB refusal source; hold the lock through /api/files/read Gate r2 Low cleanups (house rule: no aliases/shims): - Drop the is_live_database_file alias; its point-in-time caveat now lives on has_live_connection. - _refuse_live_database reuses offline_file_access's message (via _serve_offline), so a download 409 on state.db-shm names the main database like the read path; the verb is "serve" so it fits read/download/stream. - /api/files/read reads whole files in-process, so _read_base64_file now holds offline_file_access through close (409 on a live DB; OSError stays 500). Only the streamed FileResponse routes keep the point-in-time check. - That check takes the global _live_lock, which other threads hold across whole-file reads, so fs_download and the managed stream routes run it via asyncio.to_thread instead of stalling the event loop. - _managed_readable_file docstring no longer claims a size cap; _read_file_reference returns (early, text) instead of a str|Expansion union sniffed with isinstance. Co-authored-by: Benjamin PERRY --- agent/context_references.py | 19 +++++---- hermes_cli/sqlite_safe_read.py | 13 ++----- hermes_cli/web_routers/files.py | 47 ++++++++++++++--------- tests/agent/test_live_db_preview_locks.py | 17 ++++---- 4 files changed, 52 insertions(+), 44 deletions(-) diff --git a/agent/context_references.py b/agent/context_references.py index c44c5327ef..894885b3b1 100644 --- a/agent/context_references.py +++ b/agent/context_references.py @@ -302,22 +302,25 @@ def _expand_path_reference(ref: ContextReference, cwd: Path, *, allowed_root: Pa # 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"): - raw = _read_file_reference(ref, path, max_inline_tokens) + early, text = _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) + return early or _format_file_reference(ref, path, text, max_inline_tokens) -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.""" +def _read_file_reference( + ref: ContextReference, path: Path, max_inline_tokens: int | None, +) -> tuple[Expansion | None, str]: + """Raw file I/O for an @file ref: ``(early, text)`` where ``early`` is a refusal block + (then ``text`` is empty) or ``None`` with the text to inline.""" 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. - return None, _binary_reference_block(ref, path) + return (None, _binary_reference_block(ref, path)), "" if ref.line_start is not None: # A ranged ref wants a slice, not the file: stream to the window so a GB-scale # file serves :1-5 without being materialized. Lines are read in bounded pieces @@ -356,7 +359,7 @@ def _read_file_reference(ref: ContextReference, path: Path, max_inline_tokens: i break total_chars += len(line) if char_budget is not None and total_chars > char_budget: - return None, _oversized_text_reference_block(ref, path, total_chars // CHARS_PER_TOKEN) + return (None, _oversized_text_reference_block(ref, path, total_chars // CHARS_PER_TOKEN)), "" parts.append(line) text = "".join(parts) else: @@ -364,9 +367,9 @@ def _read_file_reference(ref: ContextReference, path: Path, max_inline_tokens: i # file past that byte ceiling is certainly oversized; refuse without reading it. size = path.stat().st_size 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) + return (None, _oversized_text_reference_block(ref, path, size // CHARS_PER_TOKEN)), "" text = path.read_text(encoding="utf-8-sig") - return text + return None, text def _format_file_reference(ref: ContextReference, path: Path, text: str, max_inline_tokens: int | None) -> Expansion: diff --git a/hermes_cli/sqlite_safe_read.py b/hermes_cli/sqlite_safe_read.py index 3cfaaf443a..cccdc60ea4 100644 --- a/hermes_cli/sqlite_safe_read.py +++ b/hermes_cli/sqlite_safe_read.py @@ -90,19 +90,14 @@ def _live_main_key(key: str) -> Optional[str]: def has_live_connection(path: Path | str) -> bool: - """Whether this process holds a connection to *path* (or to the database it is a sidecar of).""" + """Whether this process holds a connection to *path* (or to the database it is a sidecar of). + + Point-in-time answer: a raw open/close right after it returns ``False`` can still race a + new connection. Hold :func:`offline_file_access` across the I/O whenever possible.""" with _live_lock: 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: """Untrack-on-close behaviour, mixable into any Connection subclass. diff --git a/hermes_cli/web_routers/files.py b/hermes_cli/web_routers/files.py index 8581edbb75..fc5b953447 100644 --- a/hermes_cli/web_routers/files.py +++ b/hermes_cli/web_routers/files.py @@ -25,7 +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.sqlite_safe_read import LiveConnectionError, 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, @@ -175,18 +175,26 @@ def _fs_regular_file(path: Path) -> tuple[Path, os.stat_result]: return target, st +@contextlib.contextmanager +def _serve_offline(target: Path): + """Hold ``offline_file_access`` for serving ``target``; 409 while a SQLite connection to it is live.""" + try: + with offline_file_access(target, what="serve"): + yield + except LiveConnectionError as exc: + raise HTTPException(status_code=409, detail=str(exc)) from exc + + def _refuse_live_database(target: Path) -> None: - """409 before streaming ``target`` while this process has a live SQLite connection to it. + """Point-in-time 409 before streaming ``target`` while a SQLite connection to it is live. ``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.", - ) + (never across an await/yield), so streamed routes only get this admission check. It + takes the global registry lock, which other threads hold across whole-file reads, so + call it via ``asyncio.to_thread``.""" + with _serve_offline(target): + pass def _fs_read_bytes(target: Path, limit: Optional[int] = None) -> bytes: @@ -263,8 +271,10 @@ def _media_serve_roots() -> list[Path]: def _read_base64_file(path: Path) -> str: - """Read and encode a bounded file from a worker thread.""" - return base64.b64encode(path.read_bytes()).decode("ascii") + """Read and encode a bounded file from a worker thread; 409 while a SQLite connection to it is live.""" + # Keep admission through close; a raw close cancels this process's SQLite locks. + with _serve_offline(path): + return base64.b64encode(path.read_bytes()).decode("ascii") @router.get("/api/media") @@ -408,8 +418,9 @@ async def list_managed_files(request: Request, path: Optional[str] = None): def _managed_readable_file(request: Request, path: str) -> tuple[Any, Path, str, int, str]: """Resolve + guard a managed file for reading: existence, regular file, - sensitive-path denylist, size cap. Returns (policy, target, display_path, - size, mime_type).""" + sensitive-path denylist. Returns (policy, target, display_path, max_bytes, + mime_type). Callers own the live-SQLite 409 (held through the read for + /api/files/read, point-in-time for streamed responses).""" from hermes_cli.web_server import _MANAGED_FILE_MAX_BYTES policy, target, display_path = _resolve_managed_path(path, request) if not target.exists(): @@ -418,7 +429,6 @@ 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 @@ -449,7 +459,7 @@ async def read_managed_file(request: Request, path: str): } -def _managed_file_response( +async def _managed_file_response( request: Request, path: str, *, @@ -461,6 +471,7 @@ def _managed_file_response( if media_only and target.suffix.lower() not in _STREAMABLE_MEDIA_EXTENSIONS: raise HTTPException(status_code=415, detail="Unsupported media type") _managed_file_size(target, max_bytes) + await asyncio.to_thread(_refuse_live_database, target) return FileResponse( path=str(target), media_type=mime_type, @@ -482,7 +493,7 @@ async def download_managed_file(request: Request, path: str): """ fetch_destination = request.headers.get("sec-fetch-dest", "").lower() is_media_subresource = fetch_destination in {"audio", "video"} - return _managed_file_response( + return await _managed_file_response( request, path, content_disposition_type="inline" if is_media_subresource else "attachment", @@ -497,7 +508,7 @@ async def stream_managed_file(request: Request, path: str): media pipeline may reject an attachment response as an ``